From 5233b6e70b2704741749f3e7dcc6ce02118f6bb2 Mon Sep 17 00:00:00 2001 From: Bryan Green Date: Thu, 18 Dec 2025 13:24:39 -0600 Subject: [PATCH v3] Allow complex data for GUC extra data String GUC check hooks that allocate "extra" data previously used guc_malloc() and manual cleanup. If an ERROR was thrown during validation, cleanup code was bypassed, leaking memory in non-transactional contexts (SIGHUP reload, postmaster startup). Convert 3 check hooks to use this infrastructure, removing manual cleanup code and fixing potential memory leaks. Note: This changes OOM behavior from soft failure (return false) to hard failure (ERROR). --- src/backend/commands/tablespace.c | 10 +- src/backend/postmaster/postmaster.c | 2 + src/backend/replication/slot.c | 11 +-- src/backend/replication/syncrep.c | 20 +--- src/backend/utils/misc/guc.c | 113 ++++++++++++++++++++-- src/backend/utils/misc/guc_parameters.dat | 6 +- src/include/utils/guc.h | 2 + 7 files changed, 121 insertions(+), 43 deletions(-) diff --git a/src/backend/commands/tablespace.c b/src/backend/commands/tablespace.c index df31eace47..9a6a8a6429 100644 --- a/src/backend/commands/tablespace.c +++ b/src/backend/commands/tablespace.c @@ -1208,8 +1208,6 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source) { /* syntax error in name list */ GUC_check_errdetail("List syntax is invalid."); - pfree(rawname); - list_free(namelist); return false; } @@ -1284,19 +1282,15 @@ check_temp_tablespaces(char **newval, void **extra, GucSource source) } /* Now prepare an "extra" struct for assign_temp_tablespaces */ - myextra = guc_malloc(LOG, offsetof(temp_tablespaces_extra, tblSpcs) + + myextra = (temp_tablespaces_extra *) palloc(offsetof(temp_tablespaces_extra, tblSpcs) + numSpcs * sizeof(Oid)); - if (!myextra) - return false; myextra->numSpcs = numSpcs; memcpy(myextra->tblSpcs, tblSpcs, numSpcs * sizeof(Oid)); *extra = myextra; - pfree(tblSpcs); + } - pfree(rawname); - list_free(namelist); return true; } diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index cf44a67718..347fd9a744 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -785,6 +785,7 @@ PostmasterMain(int argc, char *argv[]) if (!SelectConfigFiles(userDoption, progname)) ExitPostmaster(2); + CleanupTempCheckHookContext(); if (output_config_variable != NULL) { /* @@ -2014,6 +2015,7 @@ process_pm_reload_request(void) ereport(LOG, (errmsg("received SIGHUP, reloading configuration files"))); ProcessConfigFile(PGC_SIGHUP); + CleanupTempCheckHookContext(); SignalChildren(SIGHUP, btmask_all_except(B_DEAD_END_BACKEND)); /* Reload authentication config files too */ diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 58c41d4551..61f584cb9e 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -2962,21 +2962,14 @@ check_synchronized_standby_slots(char **newval, void **extra, GucSource source) ok = validate_sync_standby_slots(rawname, &elemlist); if (!ok || elemlist == NIL) - { - pfree(rawname); - list_free(elemlist); return ok; - } /* Compute the size required for the SyncStandbySlotsConfigData struct */ size = offsetof(SyncStandbySlotsConfigData, slot_names); foreach_ptr(char, slot_name, elemlist) size += strlen(slot_name) + 1; - /* GUC extra value must be guc_malloc'd, not palloc'd */ - config = (SyncStandbySlotsConfigData *) guc_malloc(LOG, size); - if (!config) - return false; + config = (SyncStandbySlotsConfigData *) palloc(size); /* Transform the data into SyncStandbySlotsConfigData */ config->nslotnames = list_length(elemlist); @@ -2990,8 +2983,6 @@ check_synchronized_standby_slots(char **newval, void **extra, GucSource source) *extra = config; - pfree(rawname); - list_free(elemlist); return true; } diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index 298a3766d7..0450a2e56b 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -1060,7 +1060,6 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source) { yyscan_t scanner; int parse_rc; - SyncRepConfigData *pconf; /* Result of parsing is returned in one of these two variables */ SyncRepConfigData *syncrep_parse_result = NULL; @@ -1089,22 +1088,13 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source) return false; } - /* GUC extra value must be guc_malloc'd, not palloc'd */ - pconf = (SyncRepConfigData *) - guc_malloc(LOG, syncrep_parse_result->config_size); - if (pconf == NULL) - return false; - memcpy(pconf, syncrep_parse_result, syncrep_parse_result->config_size); - - *extra = pconf; - /* - * We need not explicitly clean up syncrep_parse_result. It, and any - * other cruft generated during parsing, will be freed when the - * current memory context is deleted. (This code is generally run in - * a short-lived context used for config file processing, so that will - * not be very long.) + * With GUC_EXTRA_IS_CONTEXT, the parser allocated syncrep_parse_result + * in TempCheckHookContext. We can use it directly without copying, + * as the infrastructure will reparent the context to GUCMemoryContext + * on success or delete it on failure. */ + *extra = syncrep_parse_result; } else *extra = NULL; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 935c235e1b..f227af40b9 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -199,6 +199,13 @@ static const char *const map_old_guc_names[] = { /* Memory context holding all GUC-related data */ static MemoryContext GUCMemoryContext; +/* + * Temporary context for check hook allocations with GUC_EXTRA_IS_CONTEXT. + * This is to clean up contexts during error recovery that haven't been + * reparented to GUCMemoryContext. There is at most one check operation in + * progress at a time, so we can get away with a single static context. + */ +static MemoryContext TempCheckHookContext = NULL; /* * We use a dynahash table to look up GUCs by name, or to iterate through * all the GUCs. The gucname field is redundant with gucvar->name, but @@ -756,6 +763,18 @@ extra_field_used(struct config_generic *gconf, void *extra) return false; } +static void +guc_free_value(struct config_generic *gconf, void *ptr) +{ + if (ptr == NULL) + return; + + if (gconf->flags & GUC_EXTRA_IS_CONTEXT) + MemoryContextDelete(GetMemoryChunkContext(ptr)); + else + guc_free(ptr); +} + /* * Support for assigning to an "extra" field of a GUC item. Free the prior * value if it's not referenced anywhere else in the item (including stacked @@ -771,7 +790,9 @@ set_extra_field(struct config_generic *gconf, void **field, void *newval) /* Free old value if it's not NULL and isn't referenced anymore */ if (oldval && !extra_field_used(gconf, oldval)) - guc_free(oldval); + { + guc_free_value(gconf, oldval); + } } /* @@ -2410,9 +2431,30 @@ AtEOXact_GUC(bool isCommit, int nestLevel) /* Update nesting level */ GUCNestLevel = nestLevel - 1; -} + /* Clean up any orphaned check hook context */ + if (TempCheckHookContext) + { + MemoryContextDelete(TempCheckHookContext); + TempCheckHookContext = NULL; + } +} +/* + * CleanupTempCheckHookContext: public function to clean up check hook context + * + * This is for use by postmaster and other non-transactional contexts where + * AtEOXact_GUC won't be called. + */ +void +CleanupTempCheckHookContext(void) +{ + if (TempCheckHookContext) + { + MemoryContextDelete(TempCheckHookContext); + TempCheckHookContext = NULL; + } +} /* * Start up automatic reporting of changes to variables marked GUC_REPORT. * This is executed at completion of backend startup. @@ -3926,7 +3968,7 @@ set_config_with_handle(const char *name, config_handle *handle, guc_free(newval); /* Release newextra, unless it's reset_extra */ if (newextra && !extra_field_used(record, newextra)) - guc_free(newextra); + guc_free_value(record, newextra); if (newval_different) { @@ -4026,7 +4068,7 @@ set_config_with_handle(const char *name, config_handle *handle, guc_free(newval); /* Perhaps we didn't install newextra anywhere */ if (newextra && !extra_field_used(record, newextra)) - guc_free(newextra); + guc_free_value(record, newextra); break; #undef newval @@ -4574,7 +4616,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) if (record->vartype == PGC_STRING && newval.stringval != NULL) guc_free(newval.stringval); - guc_free(newextra); + guc_free_value(record, newextra); } } else @@ -6114,7 +6156,7 @@ RestoreGUCState(void *gucstate) * in. */ Assert(gconf->stack == NULL); - guc_free(gconf->extra); + guc_free_value(gconf, gconf->extra); guc_free(gconf->last_reported); guc_free(gconf->sourcefile); switch (gconf->vartype) @@ -6136,7 +6178,7 @@ RestoreGUCState(void *gucstate) } } if (gconf->reset_extra && gconf->reset_extra != gconf->extra) - guc_free(gconf->reset_extra); + guc_free_value(gconf, gconf->reset_extra); /* Remove it from any lists it's in. */ RemoveGUCFromLists(gconf); /* Now we can reset the struct to PGS_S_DEFAULT state. */ @@ -6641,6 +6683,12 @@ static bool call_bool_check_hook(const struct config_generic *conf, bool *newval, void **extra, GucSource source, int elevel) { + /* + * TempCheckHookContext is used only by string check hooks, but we + * Assert it's not set here to catch any misuses. + */ + Assert(!TempCheckHookContext); + /* Quick success if no hook */ if (!conf->_bool.check_hook) return true; @@ -6675,6 +6723,12 @@ static bool call_int_check_hook(const struct config_generic *conf, int *newval, void **extra, GucSource source, int elevel) { + /* + * TempCheckHookContext is used only by string check hooks, but we + * Assert it's not set here to catch any misuses. + */ + Assert(!TempCheckHookContext); + /* Quick success if no hook */ if (!conf->_int.check_hook) return true; @@ -6709,6 +6763,12 @@ static bool call_real_check_hook(const struct config_generic *conf, double *newval, void **extra, GucSource source, int elevel) { + /* + * TempCheckHookContext is used only by string check hooks, but we + * Assert it's not set here to catch any misuses. + */ + Assert(!TempCheckHookContext); + /* Quick success if no hook */ if (!conf->_real.check_hook) return true; @@ -6743,12 +6803,23 @@ static bool call_string_check_hook(const struct config_generic *conf, char **newval, void **extra, GucSource source, int elevel) { + MemoryContext old_ctx = NULL; volatile bool result = true; /* Quick success if no hook */ if (!conf->_string.check_hook) return true; + if (conf->flags & GUC_EXTRA_IS_CONTEXT) + { + Assert(TempCheckHookContext == NULL); + + TempCheckHookContext = AllocSetContextCreate(CurrentMemoryContext, + "GUC string check context", + ALLOCSET_DEFAULT_SIZES); + MemoryContextSetIdentifier(TempCheckHookContext, conf->name); + old_ctx = MemoryContextSwitchTo(TempCheckHookContext); + } /* * If elevel is ERROR, or if the check_hook itself throws an elog * (undesirable, but not always avoidable), make sure we don't leak the @@ -6781,11 +6852,33 @@ call_string_check_hook(const struct config_generic *conf, char **newval, void ** } PG_CATCH(); { + if (TempCheckHookContext) + { + if (old_ctx) + MemoryContextSwitchTo(old_ctx); + MemoryContextDelete(TempCheckHookContext); + TempCheckHookContext = NULL; + } guc_free(*newval); PG_RE_THROW(); } PG_END_TRY(); + if (TempCheckHookContext) + { + MemoryContextSwitchTo(old_ctx); + + if (result && *extra) + { + MemoryContextSetParent(TempCheckHookContext, GUCMemoryContext); + } + else + { + MemoryContextDelete(TempCheckHookContext); + } + TempCheckHookContext = NULL; + } + return result; } @@ -6793,6 +6886,12 @@ static bool call_enum_check_hook(const struct config_generic *conf, int *newval, void **extra, GucSource source, int elevel) { + /* + * TempCheckHookContext is used only by string check hooks, but we + * Assert it's not set here to catch any misuses. + */ + Assert(!TempCheckHookContext); + /* Quick success if no hook */ if (!conf->_enum.check_hook) return true; diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index ac0c7c36c5..4c8e19204e 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2825,7 +2825,7 @@ { name => 'synchronized_standby_slots', type => 'string', context => 'PGC_SIGHUP', group => 'REPLICATION_PRIMARY', short_desc => 'Lists streaming replication standby server replication slot names that logical WAL sender processes will wait for.', long_desc => 'Logical WAL sender processes will send decoded changes to output plugins only after the specified replication slots have confirmed receiving WAL.', - flags => 'GUC_LIST_INPUT', + flags => 'GUC_LIST_INPUT | GUC_EXTRA_IS_CONTEXT', variable => 'synchronized_standby_slots', boot_val => '""', check_hook => 'check_synchronized_standby_slots', @@ -2842,7 +2842,7 @@ { name => 'synchronous_standby_names', type => 'string', context => 'PGC_SIGHUP', group => 'REPLICATION_PRIMARY', short_desc => 'Number of synchronous standbys and list of names of potential synchronous ones.', - flags => 'GUC_LIST_INPUT', + flags => 'GUC_LIST_INPUT | GUC_EXTRA_IS_CONTEXT', variable => 'SyncRepStandbyNames', boot_val => '""', check_hook => 'check_synchronous_standby_names', @@ -2946,7 +2946,7 @@ { name => 'temp_tablespaces', type => 'string', context => 'PGC_USERSET', group => 'CLIENT_CONN_STATEMENT', short_desc => 'Sets the tablespace(s) to use for temporary tables and sort files.', long_desc => 'An empty string means use the database\'s default tablespace.', - flags => 'GUC_LIST_INPUT | GUC_LIST_QUOTE', + flags => 'GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_EXTRA_IS_CONTEXT', variable => 'temp_tablespaces', boot_val => '""', check_hook => 'check_temp_tablespaces', diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index f21ec37da8..479fecef5c 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -228,6 +228,7 @@ typedef enum 0x002000 /* can't set in PG_AUTOCONF_FILENAME */ #define GUC_RUNTIME_COMPUTED 0x004000 /* delay processing in 'postgres -C' */ #define GUC_ALLOW_IN_PARALLEL 0x008000 /* allow setting in parallel mode */ +#define GUC_EXTRA_IS_CONTEXT 0x010000 /* extra field is a MemoryContext */ #define GUC_UNIT_KB 0x01000000 /* value is in kilobytes */ #define GUC_UNIT_BLOCKS 0x02000000 /* value is in blocks */ @@ -432,6 +433,7 @@ extern void AtStart_GUC(void); extern int NewGUCNestLevel(void); extern void RestrictSearchPath(void); extern void AtEOXact_GUC(bool isCommit, int nestLevel); +extern void CleanupTempCheckHookContext(void); extern void BeginReportingGUCOptions(void); extern void ReportChangedGUCOptions(void); extern void ParseLongOption(const char *string, char **name, char **value); -- 2.52.0.windows.1