Be more wary about NULL values for GUC string variables.

get_explain_guc_options() crashed if a string GUC marked GUC_EXPLAIN
has a NULL boot_val.  Nosing around found a couple of other places
that seemed insufficiently cautious about NULL string values, although
those are likely unreachable in practice.  Add some commentary
defining the expectations for NULL values of string variables,
in hopes of forestalling future additions of more such bugs.

Xing Guo, Aleksander Alekseev, Tom Lane

Discussion: https://postgr.es/m/CACpMh+AyDx5YUpPaAgzVwC1d8zfOL4JoD-uyFDnNSa1z0EsDQQ@mail.gmail.com

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 5308896c87fbd7b92c4ae9f53fc343b3ff093469..9cd1e98d82cdf6d8e94d2787c13586ae914ee1b6 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1445,7 +1445,9 @@ check_GUC_init(struct config_generic *gconf)
            {
                struct config_string *conf = (struct config_string *) gconf;
 
-               if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0)
+               if (*conf->variable != NULL &&
+                   (conf->boot_val == NULL ||
+                    strcmp(*conf->variable, conf->boot_val) != 0))
                {
                    elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s",
                         conf->gen.name, conf->boot_val ? conf->boot_val : "<null>", *conf->variable);
@@ -5215,7 +5217,14 @@ get_explain_guc_options(int *num)
                {
                    struct config_string *lconf = (struct config_string *) conf;
 
-                   modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
+                   if (lconf->boot_val == NULL &&
+                       *lconf->variable == NULL)
+                       modified = false;
+                   else if (lconf->boot_val == NULL ||
+                            *lconf->variable == NULL)
+                       modified = true;
+                   else
+                       modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0);
                }
                break;
 
@@ -5442,7 +5451,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
            {
                struct config_string *conf = (struct config_string *) gconf;
 
-               fprintf(fp, "%s", *conf->variable);
+               if (*conf->variable)
+                   fprintf(fp, "%s", *conf->variable);
            }
            break;
 

diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index d5a08806782fe39dceb4e3b26a92ff2d07f714d6..ab880cae2c410955f66551190618643066ffaad6 100644 (file)
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -240,6 +240,16 @@ struct config_real
    void       *reset_extra;
 };
 
+/*
+ * A note about string GUCs: the boot_val is allowed to be NULL, which leads
+ * to the reset_val and the actual variable value (*variable) also being NULL.
+ * However, there is no way to set a NULL value subsequently using
+ * set_config_option or any other GUC API.  Also, GUC APIs such as SHOW will
+ * display a NULL value as an empty string.  Callers that choose to use a NULL
+ * boot_val should overwrite the setting later in startup, or else be careful
+ * that NULL doesn't have semantics that are visibly different from an empty
+ * string.
+ */
 struct config_string
 {
    struct config_generic gen;