Fix small psql slash option leaks
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253383psql -h localhost -U postgresBuilt from patchset v9 (message #9), August 20, 2026 at 06:21 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253383_9 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253383_9 && git checkout t253383_9Patchset v9 (message #9) is on t253383_9
Hi,
Attached is a small patch that fixes a few cases where the result of
psql_scan_slash_option() was not freed.
psql_scan_slash_option() returns a malloc'd string, and most callers
either free it directly or transfer ownership to longer-lived psql
state. But there seems a few exceptions.
The patch covers the meta-commands: \getresults, \gset in pipeline mode,
\restrict, and \unrestrict.
I don't think these are severe leaks. The leaked object is just the
string returned by psql_scan_slash_option(), so the amount is small in
typical use and is reclaimed when the psql process exits. However, the
leak can accumulate in a long-running interactive psql session if these
are repeatedly exercised. Since nearby callers generally free
these strings explicitly, it seems worthwhile to make these cases
consistent as well.
I'm thinking of backpatching this to all supported versions. In v17 and
older, only the \restrict and \unrestrict cases apply, so those
branches would need only that part of the fix.
Thought?
Regards,
--
Fujii Masao
On Wed, Aug 12, 2026 at 12:12 PM Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Attached is a small patch that fixes a few cases where the result of
psql_scan_slash_option() was not freed.
BTW, while working on this, I found another issue around \getresults.
exec_command_getresults() sets pset.send_mode to
PSQL_SEND_GET_RESULTS before validating the optional argument. If the
argument is invalid, it returns PSQL_CMD_ERROR, so MainLoop
does not call SendQuery(). So, this send_mode state is not cleared and
remains set until the next command.
This can cause the next SQL command to behave unexpectedly, because it
is treated as a request to fetch pending results rather than as a normal
query. For example:
=# \startpipeline
=# SELECT 1;
=# \flushrequest
=# \getresults -1
\getresults: invalid number of requested results
=# SELECT 99;
?column?
----------
1
After the invalid \getresults, the following SELECT 99 is not
executed as expected. Instead, the stale PSQL_SEND_GET_RESULTS state
causes psql to fetch the pending result from the previous SELECT 1.
I'm thinking to address this separately as a follow-up after committing the
memory leak fix.
Regards,
--
Fujii Masao
On Aug 12, 2026, at 11:12, Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Attached is a small patch that fixes a few cases where the result of
psql_scan_slash_option() was not freed.psql_scan_slash_option() returns a malloc'd string, and most callers
either free it directly or transfer ownership to longer-lived psql
state. But there seems a few exceptions.The patch covers the meta-commands: \getresults, \gset in pipeline mode,
\restrict, and \unrestrict.I don't think these are severe leaks. The leaked object is just the
string returned by psql_scan_slash_option(), so the amount is small in
typical use and is reclaimed when the psql process exits. However, the
leak can accumulate in a long-running interactive psql session if these
are repeatedly exercised. Since nearby callers generally free
these strings explicitly, it seems worthwhile to make these cases
consistent as well.I'm thinking of backpatching this to all supported versions. In v17 and
older, only the \restrict and \unrestrict cases apply, so those
branches would need only that part of the fix.Thought?
+1
I never restart psql unless I’m making changes to psql itself. After restarting the server, I usually just use \c to reconnect. So, avoiding accumulated leaks is preferable.
Regards,
--
Fujii Masao
<v1-0001-Fix-psql-slash-option-leaks.patch>
V1 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Hi Fujii,
On Wed, Aug 12, 2026 at 11:12 AM Fujii Masao <masao.fujii@gmail.com> wrote:
Hi,
Attached is a small patch that fixes a few cases where the result of
psql_scan_slash_option() was not freed.psql_scan_slash_option() returns a malloc'd string, and most callers
either free it directly or transfer ownership to longer-lived psql
state. But there seems a few exceptions.The patch covers the meta-commands: \getresults, \gset in pipeline mode,
\restrict, and \unrestrict.I don't think these are severe leaks. The leaked object is just the
string returned by psql_scan_slash_option(), so the amount is small in
typical use and is reclaimed when the psql process exits. However, the
leak can accumulate in a long-running interactive psql session if these
are repeatedly exercised. Since nearby callers generally free
these strings explicitly, it seems worthwhile to make these cases
consistent as well.I'm thinking of backpatching this to all supported versions. In v17 and
older, only the \restrict and \unrestrict cases apply, so those
branches would need only that part of the fix.Thought?
Regards,
--
Fujii Masao
Could psql_scan_slash_option() return the static oom_buffer when
initPQExpBuffer() fails while parsing a quoted option? If so, the
caller's free() would be invalid.
--
Regards
Junwang Zhao
On Wed, Aug 12, 2026 at 9:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
Could psql_scan_slash_option() return the static oom_buffer when
initPQExpBuffer() fails while parsing a quoted option? If so, the
caller's free() would be invalid.
Yes, you're right! On out-of-memory, psql_scan_slash_option() can return
the static oom_buffer, so a caller that frees the returned pointer can
trigger an invalid free. I found that psql_scan_slash_command() has
the same kind of issue.
This is separate from the leak fixed by the original patch. Existing
callers can already free the results of these functions, so this
should be fixed independently.
Attached are three patches:
- 0001 fixes the psql_scan_slash_command() case.
- 0002 fixes the psql_scan_slash_option() case.
- 0003 is the original leak fix, unchanged.
The first two patches make, on OOM, these functions report "out of memory"
with pg_log_error() and return NULL instead of oom_buffer, preventing
callers from passing the static buffer to free().
One limitation of 0002 is that NULL from psql_scan_slash_option() can
still mean either "no option" or OOM. I considered adding machinery to
distinguish the two cases and updating all callers accordingly, but that
seemed unnecessarily large and complex for this issue.
So, for now, I think returning NULL after reporting the out-of-memory
error is a reasonable small fix, at least for the stable branches.
Even if a caller treats the NULL as no option, the user will still see
the out-of-memory error message output via that pg_log_error().
Thoughts?
Regards,
--
Fujii Masao
Attachments:
t253383_5v2-0001-Avoid-returning-oom_buffer-from-psql-slash-comman.patchapplication/octet-stream; name=v2-0001-Avoid-returning-oom_buffer-from-psql-slash-comman.patchDownload+16-4
v2-0003-Fix-psql-slash-option-leaks.patchapplication/octet-stream; name=v2-0003-Fix-psql-slash-option-leaks.patchDownload+8-1
v2-0002-Avoid-returning-oom_buffer-from-psql-slash-option.patchapplication/octet-stream; name=v2-0002-Avoid-returning-oom_buffer-from-psql-slash-option.patchDownload+23-4
On Thu, Aug 13, 2026 at 2:03 AM Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Aug 12, 2026 at 9:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
Could psql_scan_slash_option() return the static oom_buffer when
initPQExpBuffer() fails while parsing a quoted option? If so, the
caller's free() would be invalid.Yes, you're right! On out-of-memory, psql_scan_slash_option() can return
the static oom_buffer, so a caller that frees the returned pointer can
trigger an invalid free. I found that psql_scan_slash_command() has
the same kind of issue.This is separate from the leak fixed by the original patch. Existing
callers can already free the results of these functions, so this
should be fixed independently.Attached are three patches:
- 0001 fixes the psql_scan_slash_command() case.
- 0002 fixes the psql_scan_slash_option() case.
- 0003 is the original leak fix, unchanged.The first two patches make, on OOM, these functions report "out of memory"
with pg_log_error() and return NULL instead of oom_buffer, preventing
callers from passing the static buffer to free().One limitation of 0002 is that NULL from psql_scan_slash_option() can
still mean either "no option" or OOM. I considered adding machinery to
distinguish the two cases and updating all callers accordingly, but that
seemed unnecessarily large and complex for this issue.
Yeah, \setenv NAME VALUE can interpret an OOM while parsing VALUE
as an omitted value and unset NAME.
So, for now, I think returning NULL after reporting the out-of-memory
error is a reasonable small fix, at least for the stable branches.
Even if a caller treats the NULL as no option, the user will still see
the out-of-memory error message output via that pg_log_error().
WFM.
Thoughts?
Regards,
--
Fujii Masao
--
Regards
Junwang Zhao
On Aug 13, 2026, at 02:03, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Aug 12, 2026 at 9:01 PM Junwang Zhao <zhjwpku@gmail.com> wrote:
Could psql_scan_slash_option() return the static oom_buffer when
initPQExpBuffer() fails while parsing a quoted option? If so, the
caller's free() would be invalid.Yes, you're right! On out-of-memory, psql_scan_slash_option() can return
the static oom_buffer, so a caller that frees the returned pointer can
trigger an invalid free. I found that psql_scan_slash_command() has
the same kind of issue.This is separate from the leak fixed by the original patch. Existing
callers can already free the results of these functions, so this
should be fixed independently.Attached are three patches:
- 0001 fixes the psql_scan_slash_command() case.
- 0002 fixes the psql_scan_slash_option() case.
- 0003 is the original leak fix, unchanged.The first two patches make, on OOM, these functions report "out of memory"
with pg_log_error() and return NULL instead of oom_buffer, preventing
callers from passing the static buffer to free().One limitation of 0002 is that NULL from psql_scan_slash_option() can
still mean either "no option" or OOM. I considered adding machinery to
distinguish the two cases and updating all callers accordingly, but that
seemed unnecessarily large and complex for this issue.So, for now, I think returning NULL after reporting the out-of-memory
error is a reasonable small fix, at least for the stable branches.
Even if a caller treats the NULL as no option, the user will still see
the out-of-memory error message output via that pg_log_error().Thoughts?
I think it’s OK. As this change is for the psql client, OOM should be rare. Even if it occurs, an error message is printed, so users will be able to see it.
My only concern was whether pg_log_error() could still print the message when OOM occurs, but after checking the code, I don’t think that is a problem.
Regards,
--
Fujii Masao
<v2-0001-Avoid-returning-oom_buffer-from-psql-slash-comman.patch><v2-0003-Fix-psql-slash-option-leaks.patch><v2-0002-Avoid-returning-oom_buffer-from-psql-slash-option.patch>
V2 LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Thu, Aug 13, 2026 at 2:06 PM Chao Li <li.evan.chao@gmail.com> wrote:
Fujii Masao
<v2-0001-Avoid-returning-oom_buffer-from-psql-slash-comman.patch><v2-0003-Fix-psql-slash-option-leaks.patch><v2-0002-Avoid-returning-oom_buffer-from-psql-slash-option.patch>V2 LGTM.
I've pushed the patches. Thanks!
Regards,
--
Fujii Masao
On Wed, Aug 12, 2026 at 1:22 PM Fujii Masao <masao.fujii@gmail.com> wrote:
BTW, while working on this, I found another issue around \getresults.
Attached patch fixes this issue.
Regards,
--
Fujii Masao
On Wed, Aug 19, 2026 at 8:06 AM Fujii Masao <masao.fujii@gmail.com> wrote:
Attached patch fixes this issue.
The patch looks good. Thanks for the fix!
Regards,
Anthonin Bonnefoy
On Wed, Aug 19, 2026 at 3:50 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:
The patch looks good. Thanks for the fix!
Thanks for the review! I've pushed the patch.
Regards,
--
Fujii Masao
On Fri, Aug 21, 2026 at 12:38:57PM +0900, Fujii Masao wrote:
On Wed, Aug 19, 2026 at 3:50 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:The patch looks good. Thanks for the fix!
Thanks for the review! I've pushed the patch.
Thanks for d2ba580475ce. I have just noticed it, being hidden in this
thread.
Next time, if you need feedback for an issue related to something
tainted by my fingerprints, please feel free to add me in CC so as I
can look at it.
--
Michael