From: Daniel Gustafsson <daniel@yesql.se>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pm91.arapov@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL
Date: Fri, 14 Mar 2025 23:10:27 +0100
Message-ID: <E132D362-A669-4606-AFE1-B45C9DFCC141@yesql.se> (raw)
In-Reply-To: <3051193.1741961099@sss.pgh.pa.us>
References: <18845-582c6e10247377ec@postgresql.org>
<A664B6DF-7D34-4004-AC89-66B1517B3301@yesql.se>
<3051193.1741961099@sss.pgh.pa.us>
> On 14 Mar 2025, at 15:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> Why would we want FATAL here? Wouldn't it be better to return false like how
>> other check_ functions already do?
>
> Indeed. Also, a quick survey shows a lot of inconsistency in
> guc_malloc callers --- some are lazy and just use ERROR rather
> than LOG-and-return. That's probably all right for PGC_POSTMASTER
> variables (since there's no chance of continuing anyway) but
> perhaps it's worth improving elsewhere.
Turns out there was one more guc_malloc(LOG.. which didn't inspect the
returned allocation in check_synchronized_standby_slots. On top of that there
were a few non PGC_POSTMASTER check functions that could return false and let
the GUC machinery handle it if we want to be consistent.
The fix for check_createrole_self_grant should go down to v16 and the fix for
check_synchronized_standby_slots down to 17, the other ones aren't bugs today
so that would be a changed behaviour in backbranches.
--
Daniel Gustafsson
Attachments:v1-0001-Fix-guc_malloc-calls-to-check-for-OOM-and-return-.patch (application/octet-stream, 4.6K) download
From 39de8e1567ff0631ab30ac010f6393a028ecdac4 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <dgustafsson@postgresql.org>
Date: Fri, 14 Mar 2025 22:56:58 +0100
Subject: [PATCH v1] Fix guc_malloc calls to check for OOM and return false
check_createrole_self_grant and check_synchronized_standby_slots
were allocating memory on a LOG elevel without checking if the
allocation succeeded or not, which would have led to a segfault
if it didn't. The remaining calls were using the ERROR level
and relied on erroring out rather than returnin false to let
the GUC machinery handle it gracefully.
Author: Daniel Gustafsson <daniel@yesql.se>
Reported-by: Nikita <pm91.arapov@gmail.com>
Bug: #18845
Discussion: https://postgr.es/m/18845-582c6e10247377ec@postgresql.org
---
src/backend/access/transam/xlog.c | 4 +++-
src/backend/commands/user.c | 2 ++
src/backend/replication/slot.c | 2 ++
src/backend/tcop/backend_startup.c | 4 +++-
src/backend/tcop/postgres.c | 4 +++-
src/backend/utils/error/elog.c | 8 ++++++--
6 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.cindex 799fc739e18..0c4c5b9a368 100644--- a/src/backend/access/transam/xlog.c+++ b/src/backend/access/transam/xlog.c@@ -4791,7 +4791,9 @@ check_wal_consistency_checking(char **newval, void **extra, GucSource source)
list_free(elemlist);
/* assign new value */
- *extra = guc_malloc(ERROR, (RM_MAX_ID + 1) * sizeof(bool));+ *extra = guc_malloc(LOG, (RM_MAX_ID + 1) * sizeof(bool));+ if (!*extra)+ return false;
memcpy(*extra, newwalconsistency, (RM_MAX_ID + 1) * sizeof(bool));
return true;
}
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.cindex 8ae510c623b..0d638e29d00 100644--- a/src/backend/commands/user.c+++ b/src/backend/commands/user.c@@ -2566,6 +2566,8 @@ check_createrole_self_grant(char **newval, void **extra, GucSource source)
list_free(elemlist);
result = (unsigned *) guc_malloc(LOG, sizeof(unsigned));
+ if (!result)+ return false;
*result = options;
*extra = result;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.cindex 719e531eb90..646ba2a78fa 100644--- a/src/backend/replication/slot.c+++ b/src/backend/replication/slot.c@@ -2730,6 +2730,8 @@ check_synchronized_standby_slots(char **newval, void **extra, GucSource source)
/* GUC extra value must be guc_malloc'd, not palloc'd */
config = (SyncStandbySlotsConfigData *) guc_malloc(LOG, size);
+ if (!config)+ return false;
/* Transform the data into SyncStandbySlotsConfigData */
config->nslotnames = list_length(elemlist);
diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.cindex 27c0b3c2b04..a07c59ece01 100644--- a/src/backend/tcop/backend_startup.c+++ b/src/backend/tcop/backend_startup.c@@ -1078,7 +1078,9 @@ check_log_connections(char **newval, void **extra, GucSource source)
* We succeeded, so allocate `extra` and save the flags there for use by
* assign_log_connections().
*/
- *extra = guc_malloc(ERROR, sizeof(int));+ *extra = guc_malloc(LOG, sizeof(int));+ if (!*extra)+ return false;
*((int *) *extra) = flags;
return true;
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.cindex 55ab2da299b..0950a7bd1a2 100644--- a/src/backend/tcop/postgres.c+++ b/src/backend/tcop/postgres.c@@ -3617,7 +3617,9 @@ check_restrict_nonsystem_relation_kind(char **newval, void **extra, GucSource so
list_free(elemlist);
/* Save the flags in *extra, for use by the assign function */
- *extra = guc_malloc(ERROR, sizeof(int));+ *extra = guc_malloc(LOG, sizeof(int));+ if (!*extra)+ return false;
*((int *) *extra) = flags;
return true;
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.cindex 860bbd40d42..b891dab3bf6 100644--- a/src/backend/utils/error/elog.c+++ b/src/backend/utils/error/elog.c@@ -2198,7 +2198,9 @@ check_backtrace_functions(char **newval, void **extra, GucSource source)
* whitespace chars to save some memory, but it doesn't seem worth the
* trouble.
*/
- someval = guc_malloc(ERROR, newvallen + 1 + 1);+ someval = guc_malloc(LOG, newvallen + 1 + 1);+ if (!someval)+ return false;
for (i = 0, j = 0; i < newvallen; i++)
{
if ((*newval)[i] == ',')
@@ -2283,7 +2285,9 @@ check_log_destination(char **newval, void **extra, GucSource source)
pfree(rawstring);
list_free(elemlist);
- myextra = (int *) guc_malloc(ERROR, sizeof(int));+ myextra = (int *) guc_malloc(LOG, sizeof(int));+ if (!myextra)+ return false;
*myextra = newlogdest;
*extra = myextra;
--
2.39.3 (Apple Git-146)
=
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: pgsql-bugs@postgresql.org
Cc: daniel@yesql.se, tgl@sss.pgh.pa.us, pm91.arapov@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18845: DEREF_OF_NULL.RET guc_malloc possibly returns NULL
In-Reply-To: <E132D362-A669-4606-AFE1-B45C9DFCC141@yesql.se>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox