Postmaster crashes on SIGHUP when oauth_validator_libraries holds only whitespace
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.
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:t253783psql -h localhost -U postgresBuilt from patchset v7 (message #7), September 16, 2026 at 08:42 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 t253783_7 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 t253783_7 && git checkout t253783_7Patchset v7 (message #7) is on t253783_7
Hi,
I ran into a postmaster crash while investigating a static analyzer
report against 18.6. I reproduced it on master (92aaf50e230,
--enable-cassert, Debian 13/aarch64).
To reproduce, set the following in postgresql.conf:
oauth_validator_libraries = ' '
and add an OAuth line without validator= to pg_hba.conf:
host all all 127.0.0.1/32 oauth issuer="https://example.com" scope="openid"
On reload the server log ends here:
LOG: received SIGHUP, reloading configuration files
LOG: parameter "oauth_validator_libraries" changed to " "
gdb then reports SIGSEGV in check_oauth_validator(), via load_hba().
The running instance goes down with every session on it. The same
configuration also prevents startup. pg_hba_file_rules() parses the
file from a regular backend, so the same NULL dereference there kills
the backend and the postmaster restarts the cluster.
check_oauth_validator() treats an unset list as an error by looking at
the raw GUC string, but SplitDirectoriesString() accepts a string of
spaces and returns NIL. The subsequent elemlist->length dereferences
that. This is a misconfiguration rather than a security issue -- the
GUC has PGC_SIGHUP context and is marked GUC_SUPERUSER_ONLY.
I've attached a patch against master that checks the parsed list
instead. It also adds a TAP test for reload. The patched master built
without warnings, and "make check" in src/test/modules/oauth_validator
passed with PG_TEST_EXTRA=oauth.
This appears to affect v18 onward, where OAuth support was added, so it
may need back-patching. The v18 error message has different wording
and would need a small adjustment.
Could someone take a look at the attached patch and let me know
if this is the right fix?
Thanks,
Yuriy
On 14 Sep 2026, at 13:22, Grigorev Jurij <ju.grigorev@ftdata.ru> wrote:
Could someone take a look at the attached patch and let me know
if this is the right fix?
Thanks for the report, that does indeed seem like the right fix. Added to my
list of patches to review and apply before the next minors
--
Daniel Gustafsson
On Mon, Sep 14, 2026 at 4:28 AM Daniel Gustafsson <daniel@yesql.se> wrote:
Thanks for the report, that does indeed seem like the right fix.
Yes, thanks! Couple thoughts on specific pieces:
+ /* + * An empty or all-whitespace setting is accepted by + * SplitDirectoriesString(), which returns an empty list for it, so the + * parse result has to be checked rather than the raw string. + */
Sometimes recording a historical bug in the comments can help prevent
future mistakes... but I don't think this is one of those cases,
especially since the new test prevents accidental regression.
+is($bgconn->query_safe('SELECT 1'), '1', + 'postmaster survives an empty OAuth validator list on reload');
query_safe() doesn't return on failure, so IMO we shouldn't wrap it in is().
--
This seems like a good time to mention that I have a checklist item to
fix the following (shouldn't block this patch):
if (!SplitDirectoriesString(rawstring, ',', &elemlist))
...
if (strcmp(allowed, hbaline->oauth_validator) == 0)
SplitDirectoriesString() canonicalizes its outputs, which we then
compare against the uncanonicalized hbaline->oauth_validator. That
could lead to annoying false negatives in more complicated setups.
Thanks,
--Jacob
On 14 Sep 2026, at 23:28, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
+ /* + * An empty or all-whitespace setting is accepted by + * SplitDirectoriesString(), which returns an empty list for it, so the + * parse result has to be checked rather than the raw string. + */Sometimes recording a historical bug in the comments can help prevent
future mistakes... but I don't think this is one of those cases,
especially since the new test prevents accidental regression.
+1
This seems like a good time to mention that I have a checklist item to
fix the following (shouldn't block this patch):if (!SplitDirectoriesString(rawstring, ',', &elemlist))
...
if (strcmp(allowed, hbaline->oauth_validator) == 0)SplitDirectoriesString() canonicalizes its outputs, which we then
compare against the uncanonicalized hbaline->oauth_validator. That
could lead to annoying false negatives in more complicated setups.
Right, this patch wont move the needle in the wrong direction for future fixes
AFAICT.
--
Daniel Gustafsson
Hi Jacob, Daniel,
Thanks for the review! In the attached v2 I removed the explanatory
comment and call query_safe() directly. The functional change and the
reload test are otherwise unchanged.
Also left the validator-name canonicalization issue out of this patch.
Checked, the patch builds cleanly against master with --enable-cassert,
and PG_TEST_EXTRA=oauth make check in src/test/modules/oauth_validator
passes all 190 tests.
Regards,
Yuriy
________________________________________
От: Daniel Gustafsson <daniel@yesql.se>
Отправлено: 15 сентября 2026 г. 4:34:08
Кому: Jacob Champion
Копия: Григорьев Юрий; pgsql-bugs@lists.postgresql.org
Тема: Re: Postmaster crashes on SIGHUP when oauth_validator_libraries holds only whitespace
On 14 Sep 2026, at 23:28, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
+ /* + * An empty or all-whitespace setting is accepted by + * SplitDirectoriesString(), which returns an empty list for it, so the + * parse result has to be checked rather than the raw string. + */Sometimes recording a historical bug in the comments can help prevent
future mistakes... but I don't think this is one of those cases,
especially since the new test prevents accidental regression.
+1
This seems like a good time to mention that I have a checklist item to
fix the following (shouldn't block this patch):if (!SplitDirectoriesString(rawstring, ',', &elemlist))
...
if (strcmp(allowed, hbaline->oauth_validator) == 0)SplitDirectoriesString() canonicalizes its outputs, which we then
compare against the uncanonicalized hbaline->oauth_validator. That
could lead to annoying false negatives in more complicated setups.
Right, this patch wont move the needle in the wrong direction for future fixes
AFAICT.
--
Daniel Gustafsson
- if (oauth_validator_libraries_string[0] == '\0')
- {
- ereport(elevel,
- errcode(ERRCODE_CONFIG_FILE_ERROR),
- errmsg("parameter \"%s\" must be set for authentication method \"%s\"",
- "oauth_validator_libraries", "oauth"),
- errcontext("line %d of configuration file \"%s\"",
- line_num, file_name));
- *err_msg = psprintf("parameter \"%s\" must be set for authentication method \"%s\"",
- "oauth_validator_libraries", "oauth");
- return false;
- }
-
/* SplitDirectoriesString needs a modifiable copy */
rawstring = pstrdup(oauth_validator_libraries_string);
pstrdup calls strlen which segfault on NULL. oauth_validator_libraries_string
has a default value of "" so it cannot be set to NULL by user action, but the
global variable backing the GUC is initialized as NULL so I wonder if it's
worth adding defensive programming like the below, or perhaps an Assert?
/* SplitDirectoriesString needs a modifiable copy */
- rawstring = pstrdup(oauth_validator_libraries_string);
+ rawstring = pstrdup(oauth_validator_libraries_string ?
+ oauth_validator_libraries_string : "");--
Daniel Gustafsson
Hi Daniel,
You're right. I would rather not treat NULL as an empty setting, since
that would turn an internal programming error into the user-facing
"must be set" error. Attached v3 adds:
Assert(oauth_validator_libraries_string != NULL);
before the pstrdup().
I also moved the whitespace-only TAP case to after the
pg_hba_file_rules() check. Its previous position made the test racy:
wait_for_log() synchronized with the postmaster reload, but not
necessarily with the process-local GUC state of the existing bgconn.
As a result, pg_hba_file_rules() could run in a backend that still had
the whitespace value loaded and return unexpected empty fields.
Moving the case after that assertion avoids making the
pg_hba_file_rules() result depend on the timing of SIGHUP processing in
bgconn. After restoring the setting, the test runs SHOW
oauth_validator_libraries through bgconn, ensuring that the backend has
processed the second SIGHUP before the later tests continue.
Thanks,
Yuriy