From cfa4cff30c3021597473178e1c6c5592f6437119 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 12 Oct 2020 13:31:24 -0400 Subject: [PATCH] Fix memory leak when guc.c decides a setting can't be applied now. The prohibitValueChange code paths in set_config_option(), which are executed whenever we re-read a PGC_POSTMASTER variable from postgresql.conf, neglected to free anything before exiting. Thus we'd leak the proposed new value of a PGC_STRING variable, as noted by BoChen in bug #16666. For all variable types, if the check hook creates an "extra" chunk, we'd also leak that. These are malloc not palloc chunks, so there is no mechanism for recovering the leaks before process exit. Fortunately, the values are typically not very large, meaning you'd have to go through an awful lot of SIGHUP configuration-reload cycles to make the leakage amount to anything. Still, for a long-lived postmaster process it could potentially be a problem. Oversight in commit 2594cf0e8. Back-patch to all supported branches. Discussion: https://postgr.es/m/16666-2c41a4eec61b03e1@postgresql.org --- src/backend/utils/misc/guc.c | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e58f12adba..710344c72b 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -6437,6 +6437,10 @@ set_config_option(const char *name, const char *value, if (prohibitValueChange) { + /* Release newextra, unless it's reset_extra */ + if (newextra && !extra_field_used(&conf->gen, newextra)) + free(newextra); + if (*conf->variable != newval) { record->status |= GUC_PENDING_RESTART; @@ -6527,6 +6531,10 @@ set_config_option(const char *name, const char *value, if (prohibitValueChange) { + /* Release newextra, unless it's reset_extra */ + if (newextra && !extra_field_used(&conf->gen, newextra)) + free(newextra); + if (*conf->variable != newval) { record->status |= GUC_PENDING_RESTART; @@ -6617,6 +6625,10 @@ set_config_option(const char *name, const char *value, if (prohibitValueChange) { + /* Release newextra, unless it's reset_extra */ + if (newextra && !extra_field_used(&conf->gen, newextra)) + free(newextra); + if (*conf->variable != newval) { record->status |= GUC_PENDING_RESTART; @@ -6723,9 +6735,21 @@ set_config_option(const char *name, const char *value, if (prohibitValueChange) { + bool newval_different; + /* newval shouldn't be NULL, so we're a bit sloppy here */ - if (*conf->variable == NULL || newval == NULL || - strcmp(*conf->variable, newval) != 0) + newval_different = (*conf->variable == NULL || + newval == NULL || + strcmp(*conf->variable, newval) != 0); + + /* Release newval, unless it's reset_val */ + if (newval && !string_field_used(conf, newval)) + free(newval); + /* Release newextra, unless it's reset_extra */ + if (newextra && !extra_field_used(&conf->gen, newextra)) + free(newextra); + + if (newval_different) { record->status |= GUC_PENDING_RESTART; ereport(elevel, @@ -6820,6 +6844,10 @@ set_config_option(const char *name, const char *value, if (prohibitValueChange) { + /* Release newextra, unless it's reset_extra */ + if (newextra && !extra_field_used(&conf->gen, newextra)) + free(newextra); + if (*conf->variable != newval) { record->status |= GUC_PENDING_RESTART;