From 058b5152f02ef86c98a795c14dbd6a8e195f4fd1 Mon Sep 17 00:00:00 2001 From: Daniel Gustafsson Date: Thu, 27 Mar 2025 22:57:34 +0100 Subject: [PATCH] Fix guc_malloc calls for consistency and OOM checks 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 on allocation failure. On top of that, a number of callsites were using the ERROR level, relying on erroring out rather than returning false to allow the GUC machinery handle it gracefully. Other callsites used WARNING instead of LOG. While neither being not wrong, this changes all check_ functions do it consistently with LOG. init_custom_variable gets a promoted elevel to FATAL to keep the guc_malloc error handling in line with the rest of the error handling in that function which already call FATAL. If we encounter an OOM in this callsite there is no graceful handling to be had, better to error out hard. Backpatch the fix to check_createrole_self_grant down to v16 and the fix to check_synchronized_standby_slots down to v17 where they were introduced. Author: Daniel Gustafsson Reported-by: Nikita Reviewed-by: Tom Lane Bug: #18845 Discussion: https://postgr.es/m/18845-582c6e10247377ec@postgresql.org Backpatch-through: 16 --- src/backend/access/transam/xlog.c | 4 +++- src/backend/access/transam/xlogrecovery.c | 12 +++++++++--- src/backend/commands/user.c | 2 ++ src/backend/commands/variable.c | 4 ++-- src/backend/replication/slot.c | 2 ++ src/backend/storage/file/fd.c | 4 +++- src/backend/tcop/backend_startup.c | 4 +++- src/backend/tcop/postgres.c | 4 +++- src/backend/utils/error/elog.c | 8 ++++++-- src/backend/utils/misc/guc.c | 5 +++-- 10 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 4b6c694a3f7..fc30a52d496 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/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c index 2c19013c98b..0aa3ab59085 100644 --- a/src/backend/access/transam/xlogrecovery.c +++ b/src/backend/access/transam/xlogrecovery.c @@ -4833,7 +4833,9 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source) if (have_error) return false; - myextra = (XLogRecPtr *) guc_malloc(ERROR, sizeof(XLogRecPtr)); + myextra = (XLogRecPtr *) guc_malloc(LOG, sizeof(XLogRecPtr)); + if (!myextra) + return false; *myextra = lsn; *extra = myextra; } @@ -4997,7 +4999,9 @@ check_recovery_target_timeline(char **newval, void **extra, GucSource source) } } - myextra = (RecoveryTargetTimeLineGoal *) guc_malloc(ERROR, sizeof(RecoveryTargetTimeLineGoal)); + myextra = (RecoveryTargetTimeLineGoal *) guc_malloc(LOG, sizeof(RecoveryTargetTimeLineGoal)); + if (!myextra) + return false; *myextra = rttg; *extra = myextra; @@ -5033,7 +5037,9 @@ check_recovery_target_xid(char **newval, void **extra, GucSource source) if (errno == EINVAL || errno == ERANGE) return false; - myextra = (TransactionId *) guc_malloc(ERROR, sizeof(TransactionId)); + myextra = (TransactionId *) guc_malloc(LOG, sizeof(TransactionId)); + if (!myextra) + return false; *myextra = xid; *extra = myextra; } diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c index 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/commands/variable.c b/src/backend/commands/variable.c index f550a3c0c63..84f044a1959 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -1087,7 +1087,7 @@ check_application_name(char **newval, void **extra, GucSource source) if (!clean) return false; - ret = guc_strdup(WARNING, clean); + ret = guc_strdup(LOG, clean); if (!ret) { pfree(clean); @@ -1125,7 +1125,7 @@ check_cluster_name(char **newval, void **extra, GucSource source) if (!clean) return false; - ret = guc_strdup(WARNING, clean); + ret = guc_strdup(LOG, clean); if (!ret) { pfree(clean); diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 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/storage/file/fd.c b/src/backend/storage/file/fd.c index 0c3a2a756e7..84873f41488 100644 --- a/src/backend/storage/file/fd.c +++ b/src/backend/storage/file/fd.c @@ -4042,7 +4042,9 @@ check_debug_io_direct(char **newval, void **extra, GucSource source) return result; /* Save the flags in *extra, for use by assign_debug_io_direct */ - *extra = guc_malloc(ERROR, sizeof(int)); + *extra = guc_malloc(LOG, sizeof(int)); + if (!*extra) + return false; *((int *) *extra) = flags; return result; diff --git a/src/backend/tcop/backend_startup.c b/src/backend/tcop/backend_startup.c index 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.c index 4d2edb10658..aec65007bb6 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3648,7 +3648,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.c index 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; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 12192445218..667df448732 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4909,10 +4909,11 @@ init_custom_variable(const char *name, strcmp(name, "pljava.vmoptions") == 0)) context = PGC_SUSET; - gen = (struct config_generic *) guc_malloc(ERROR, sz); + /* As above, an OOM here is FATAL */ + gen = (struct config_generic *) guc_malloc(FATAL, sz); memset(gen, 0, sz); - gen->name = guc_strdup(ERROR, name); + gen->name = guc_strdup(FATAL, name); gen->context = context; gen->group = CUSTOM_OPTIONS; gen->short_desc = short_desc;