pg_stash_advice: dump file with overlong stash name crashes worker in a restart loop
Hi,
While testing pg_stash_advice, I came across a
crash that's reachable through the persistence dump file:
If $PGDATA/pg_stash_advice.tsv contains a stash name of 64 bytes or more
(i.e. >= NAMEDATALEN), the worker hits an assertion in dshash_strhash()
on startup. The postmaster treats the SIGABRT as a crash, performs full
crash recovery, restarts the worker, and the worker hits the same
assertion again. The cluster never reaches a stable state until the
file is removed by hand.
SQL-level callers cannot reach this because pgsa_check_stash_name()
rejects long names. The persistence parser, however, accepts the name
straight from the file and feeds it into dshash_find_or_insert(), which
in turn calls dshash_strhash() configured with key_size = NAMEDATALEN.
That function asserts strlen(v) < size. Strictly speaking the file
parser was missing a length check that the SQL path already performs.
Minimal reproduction
====================
$ pg_ctl -D $PGDATA stop -m fast
$ python3 -c "print('stash\t' + 'a'*64)" > $PGDATA/pg_stash_advice.tsv
$ pg_ctl -D $PGDATA -l logfile start
Server log (excerpt, every ~1-2 seconds):
DEBUG: pg_stash_advice worker started
DEBUG: restoring advice stash "aaaa...aaaa"
TRAP: failed Assert("strlen((const char *) v) < size"),
File: "dshash.c", Line: 634
postgres: pg_stash_advice worker (ExceptionalCondition+0xbb)
postgres: pg_stash_advice worker (dshash_strhash+0x48)
postgres: pg_stash_advice worker (dshash_find_or_insert_extended+0x2e)
pg_stash_advice.so (pgsa_create_stash)
pg_stash_advice.so (pgsa_restore_stashes)
pg_stash_advice.so (pgsa_read_from_disk)
pg_stash_advice.so (pg_stash_advice_worker_main+0x1aa)
...
LOG: background worker "pg_stash_advice worker" was terminated by
signal 6: Aborted
LOG: terminating any other active server processes
LOG: all server processes terminated; reinitializing
[worker comes back up, asserts again, ad infinitum]
In my testing this produced ~18 crashes in ~12 seconds before I removed
the file. Threshold is exact: 63 bytes is fine, 64 bytes crashes.
In a production (non-assertion) build the name would instead be silently
truncated to NAMEDATALEN-1 bytes by the dshash key-copy path, allowing
distinct long names that share a 63-byte prefix to collide in the stash
table.
The attached patch validates the parsed stash-name length in the same
place where the parser already raises ERRCODE_DATA_CORRUPTED on other
forms of syntax error, mirroring the existing length check in
pgsa_check_stash_name(). Not sure if TAP test too is needed for this, but
I've included one in the patch.
Regards,
Ayush
Attachments:
0001-pg_stash_advice-reject-overlong-stash-names-when-loa.patchapplication/octet-stream; name=0001-pg_stash_advice-reject-overlong-stash-names-when-loa.patchDownload+36-1
On Sun, May 17, 2026 at 2:25 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
While testing pg_stash_advice, I came across a
crash that's reachable through the persistence dump file:
Hi,
Thanks for the report and patch. I found it to be more elaborate than
what I think we need, so I have committed a simplified version. While
it's good to fix stuff like this, the chances of anyone hitting in
practice are very low, since it requires the dump file to be
corrupted. Adding a test that could fail with a different value of
NAMEDATALEN seems likely to hurt more than it helps, and making the
test more elaborate to avoid that peril does not seem like the best
use of time and energy. I don't see this as something that needs to be
tested on every regression test run from now until forever.
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
On Fri, 29 May 2026 at 22:55, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, May 17, 2026 at 2:25 AM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:While testing pg_stash_advice, I came across a
crash that's reachable through the persistence dump file:Hi,
Thanks for the report and patch. I found it to be more elaborate than
what I think we need, so I have committed a simplified version. While
it's good to fix stuff like this, the chances of anyone hitting in
practice are very low, since it requires the dump file to be
corrupted. Adding a test that could fail with a different value of
NAMEDATALEN seems likely to hurt more than it helps, and making the
test more elaborate to avoid that peril does not seem like the best
use of time and energy. I don't see this as something that needs to be
tested on every regression test run from now until forever.
Thanks for the update and fix!
While reading through pg_stash_advice I noticed something about
pg_set_stashed_advice(stash_name text, query_id bigint, advice_string text)
that I wanted to ask about, since I might be misunderstanding the intended
design.
The function is (correctly, I think) not marked STRICT, because a NULL
advice_string is meaningful, it clears any existing advice for that
stash/query ID. That part makes sense to me.
What surprised me is the handling of the first two arguments:
if (PG_ARGISNULL(0) || PG_ARGISNULL(1))
PG_RETURN_NULL();
So a NULL stash name or NULL query ID is silently accepted and the call
just becomes a no-op that returns NULL. That felt a little odd next to the
neighboring check, where a query ID of 0 is treated as a hard error:
if (queryId == 0)
ereport(ERROR, ... "cannot set advice string for query ID 0");
A few thing I was wondering about:
1. Is the PG_RETURN_NULL() on NULL stash name / query ID intentional, or is
it a leftover from an earlier shape of the function? It looks a bit like
an attempt to emulate STRICT for just the first two arguments, but the
effect is that an accidental NULL (say, a scalar subquery that returned
no rows) silently does nothing rather than telling the user something
went wrong.
2. While there, I noticed the function's header comment says "If the second
argument is NULL, we delete any existing advice stash entry", but the
code actually keys the delete path off the third argument (advice_string,
PG_ARGISNULL(2)). I think the comment is just stale, does that match
your understanding?
Regards,
Ayush
On Fri, May 29, 2026 at 3:43 PM Ayush Tiwari
<ayushtiwari.slg01@gmail.com> wrote:
1. Is the PG_RETURN_NULL() on NULL stash name / query ID intentional, or is
it a leftover from an earlier shape of the function? It looks a bit like
an attempt to emulate STRICT for just the first two arguments, but the
effect is that an accidental NULL (say, a scalar subquery that returned
no rows) silently does nothing rather than telling the user something
went wrong.
It's intentional, and your guess about emulating STRICT functions is
correct. Of course, this function could choose to throw an ERROR in
such cases, and that wouldn't be wrong, but I couldn't think of a
reason to do it that way, so I didn't.
2. While there, I noticed the function's header comment says "If the second
argument is NULL, we delete any existing advice stash entry", but the
code actually keys the delete path off the third argument (advice_string,
PG_ARGISNULL(2)). I think the comment is just stale, does that match
your understanding?
Yes, I suppose that needs to say "third".
--
Robert Haas
EDB: http://www.enterprisedb.com