public inbox for [email protected]
help / color / mirror / Atom feedFrom: Heikki Linnakangas <[email protected]>
To: Lukas Fittl <[email protected]>
To: Gaurav Singh <[email protected]>
Cc: [email protected]
Subject: Re: Memory leak in pg_stat_statements when qtext file contains invalid encoding
Date: Fri, 27 Mar 2026 10:59:32 +0200
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAP53Pky1575W1euSUwj2Zc5b0kBFZqpNuvMOGCvU_bR1tmD7zA@mail.gmail.com>
References: <CAEcQ1bYR9s4eQLFDjzzJHU8fj-MTbmRpW-9J-r2gsCn+HEsynw@mail.gmail.com>
<CAP53Pky1575W1euSUwj2Zc5b0kBFZqpNuvMOGCvU_bR1tmD7zA@mail.gmail.com>
On 27/03/2026 10:21, Lukas Fittl wrote:
> Hi Gaurav,
>
> On Fri, Mar 27, 2026 at 12:54 AM Gaurav Singh <[email protected]> wrote:
>> If the qtext file contains an invalid encoding, pg_any_to_server calls ereport(ERROR) which longjmps out of the function.
>> The cleanup code at the bottom of the function is never reached.
>>
>> LWLockRelease(pgss->lock);
>> if (qbuffer)
>> free(qbuffer);
>> On every subsequent call, the malloc'd buffer (the entire file contents) is leaked, and the LWLock release is also skipped.
>
> I don't think the analysis is correct in regards to the LWLock release
> - that should be taken care of by LWLockReleaseAll on abort.
>
> But I think you're correct about qbuffer - because that buffer is
> using malloc (not palloc), its not part of any memory context, and so
> it will happily leak on abort.
Yep
> It appears our use of malloc in pg_stat_statements is so that we can
> fail on OOM and return NULL without a jump. I think that makes sense
> for when a GC cycle was triggered during regular query execution
> (since we don't want to error the original query), but it seems like
> just bubbling up the OOM if needed when querying the
> pg_stat_statements function seems fine.
>
> I wonder if its worth separating the two cases, since the issue you're
> describing (the call to pg_any_to_server failing) only happens when
> returning the query text file contents to the client. I think your
> PG_FINALLY suggestion could also work, but it feels a bit tedious to
> wrap the whole pg_stat_statements_internal function in it.
Hmm, perhaps. But there's a simpler, less invasive fix. When that code
was written, we didn't have MCXT_ALLOC_HUGE nor MCXT_ALLOC_NO_OOM. Now
that we do, we can just use palloc_extended(MCXT_ALLOC_HUGE |
MCXT_ALLOC_NO_OOM) instead of raw malloc(). Per attached.
- Heikki
Attachments:
[text/x-patch] 0001-Avoid-memory-leak-on-error-while-parsing-pg_stat_sta.patch (3.1K, 2-0001-Avoid-memory-leak-on-error-while-parsing-pg_stat_sta.patch)
download | inline diff:
From a88fdd79e0131a25141f5efd84246026b3b7f6ff Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Fri, 27 Mar 2026 10:52:04 +0200
Subject: [PATCH 1/1] Avoid memory leak on error while parsing
pg_stat_statements dump file
By using palloc() instead of raw malloc().
Reported-by: Gaurav Singh <[email protected]>
Reviewed-by: Lukas Fittl <[email protected]>
Discussion: https://www.postgresql.org/message-id/CAEcQ1bYR9s4eQLFDjzzJHU8fj-MTbmRpW-9J-r2gsCn+HEsynw@mail.gmail.com
---
.../pg_stat_statements/pg_stat_statements.c | 22 +++++++++++--------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6cb14824ec3..7975476b890 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -805,7 +805,7 @@ pgss_shmem_shutdown(int code, Datum arg)
if (fwrite(&pgss->stats, sizeof(pgssGlobalStats), 1, file) != 1)
goto error;
- free(qbuffer);
+ pfree(qbuffer);
qbuffer = NULL;
if (FreeFile(file))
@@ -829,7 +829,8 @@ error:
(errcode_for_file_access(),
errmsg("could not write file \"%s\": %m",
PGSS_DUMP_FILE ".tmp")));
- free(qbuffer);
+ if (qbuffer)
+ pfree(qbuffer);
if (file)
FreeFile(file);
unlink(PGSS_DUMP_FILE ".tmp");
@@ -1825,7 +1826,8 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
pgss->extent != extent ||
pgss->gc_count != gc_count)
{
- free(qbuffer);
+ if (qbuffer)
+ pfree(qbuffer);
qbuffer = qtext_load_file(&qbuffer_size);
}
}
@@ -2046,7 +2048,8 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
LWLockRelease(pgss->lock);
- free(qbuffer);
+ if (qbuffer)
+ pfree(qbuffer);
}
/* Number of output arguments (columns) for pg_stat_statements_info */
@@ -2333,7 +2336,7 @@ error:
}
/*
- * Read the external query text file into a malloc'd buffer.
+ * Read the external query text file into a palloc'd buffer.
*
* Returns NULL (without throwing an error) if unable to read, eg
* file not there or insufficient memory.
@@ -2375,7 +2378,7 @@ qtext_load_file(Size *buffer_size)
/* Allocate buffer; beware that off_t might be wider than size_t */
if (stat.st_size <= MaxAllocHugeSize)
- buf = (char *) malloc(stat.st_size);
+ buf = (char *) palloc_extended(stat.st_size, MCXT_ALLOC_HUGE | MCXT_ALLOC_NO_OOM);
else
buf = NULL;
if (buf == NULL)
@@ -2414,7 +2417,7 @@ qtext_load_file(Size *buffer_size)
(errcode_for_file_access(),
errmsg("could not read file \"%s\": %m",
PGSS_TEXT_FILE)));
- free(buf);
+ pfree(buf);
CloseTransientFile(fd);
return NULL;
}
@@ -2625,7 +2628,7 @@ gc_qtexts(void)
else
pgss->mean_query_len = ASSUMED_LENGTH_INIT;
- free(qbuffer);
+ pfree(qbuffer);
/*
* OK, count a garbage collection cycle. (Note: even though we have
@@ -2642,7 +2645,8 @@ gc_fail:
/* clean up resources */
if (qfile)
FreeFile(qfile);
- free(qbuffer);
+ if (qbuffer)
+ pfree(qbuffer);
/*
* Since the contents of the external file are now uncertain, mark all
--
2.47.3
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Memory leak in pg_stat_statements when qtext file contains invalid encoding
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox