Thread: Tightening up allowed custom GUC names

Over in [1] it was noted that the system behaves rather oddly if
you try to do ALTER USER/DATABASE SET with a custom GUC name
containing "=" or "-".  I think we should just disallow such cases.
Relaxing the restriction is harder than it might seem:

* The convention for entries in pg_db_role_setting is just
"name=value" with no quoting rule, so GUC names containing "="
can't work.  We could imagine installing some kind of quoting rule,
but that would break client-side code that looks at this catalog;
pg_dump, for one, does so.  On balance it seems clearly not worth
changing that.

* The problem with using "-" is that we parse pg_db_role_setting
entries with ParseLongOption(), which converts "-" to "_" because
that's what makes sense to do in the context of command-line switches
such as "-c work-mem=42MB".  We could imagine adjusting the code to
not do that in the pg_db_role_setting case, but you'd still be left
with a GUC that cannot be set via PGOPTIONS="-c custom.my-guc=42".
To avoid that potential confusion, it seems best to ban "-" as well
as "=".

Now granting that the best answer is just to forbid these cases,
there are still a couple of decisions about how extensive the
prohibition ought to be:

* We could forbid these characters only when you try to actually
put such a GUC into pg_db_role_setting, and otherwise allow them.
That seems like a weird nonorthogonal choice though, so I'd
rather just forbid them period.

* A case could be made for tightening things up a lot more, and not
allowing anything that doesn't look like an identifier.  I'm not
pushing for that, as it seems more likely to break existing
applications than the narrow restriction proposed here.  But I could
live with it if people prefer that way.

Anyway, attached is a proposed patch that implements the restriction
as stated.  I'm inclined to propose this for HEAD only and not
worry about the issue in the back branches.

Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/20210209144059.GA21360%40depesz.com

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 7885a169bb..43cf0c4434 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -282,7 +282,7 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
          * Try to find the variable; but do not create a custom placeholder if
          * it's not there already.
          */
-        record = find_option(item->name, false, elevel);
+        record = find_option(item->name, false, true, elevel);

         if (record)
         {
@@ -306,7 +306,8 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
             /* Now mark it as present in file */
             record->status |= GUC_IS_IN_FILE;
         }
-        else if (strchr(item->name, GUC_QUALIFIER_SEPARATOR) == NULL)
+        else if (strchr(item->name, GUC_QUALIFIER_SEPARATOR) == NULL ||
+                 strcspn(item->name, GUC_DISALLOWED_NAME_CHARS) != strlen(item->name))
         {
             /* Invalid non-custom variable, so complain */
             ereport(elevel,
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index eafdb1118e..959099122b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5289,12 +5289,23 @@ add_placeholder_variable(const char *name, int elevel)
 }

 /*
- * Look up option NAME.  If it exists, return a pointer to its record,
- * else return NULL.  If create_placeholders is true, we'll create a
- * placeholder record for a valid-looking custom variable name.
+ * Look up option "name".  If it exists, return a pointer to its record.
+ * Otherwise, if create_placeholders is true and name is a valid-looking
+ * custom variable name, we'll create and return a placeholder record.
+ * Otherwise, if skip_errors is true, then we silently return NULL for
+ * an unrecognized or invalid name.  Otherwise, the error is reported at
+ * error level elevel (and we return NULL if that's less than ERROR).
+ *
+ * Note: internal errors, primarily out-of-memory, draw an elevel-level
+ * report and NULL return regardless of skip_errors.  Hence, callers must
+ * handle a NULL return whenever elevel < ERROR, but they should not need
+ * to emit any additional error message.  (In practice, internal errors
+ * can only happen when create_placeholders is true, so callers passing
+ * false need not think terribly hard about this.)
  */
 static struct config_generic *
-find_option(const char *name, bool create_placeholders, int elevel)
+find_option(const char *name, bool create_placeholders, bool skip_errors,
+            int elevel)
 {
     const char **key = &name;
     struct config_generic **res;
@@ -5322,19 +5333,38 @@ find_option(const char *name, bool create_placeholders, int elevel)
     for (i = 0; map_old_guc_names[i] != NULL; i += 2)
     {
         if (guc_name_compare(name, map_old_guc_names[i]) == 0)
-            return find_option(map_old_guc_names[i + 1], false, elevel);
+            return find_option(map_old_guc_names[i + 1], false,
+                               skip_errors, elevel);
     }

     if (create_placeholders)
     {
         /*
-         * Check if the name is qualified, and if so, add a placeholder.
+         * Check if the name is valid, and if so, add a placeholder.  To be
+         * allowed, it must contain a separator character, but not any of the
+         * disallowed characters.
          */
         if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL)
-            return add_placeholder_variable(name, elevel);
+        {
+            if (strcspn(name, GUC_DISALLOWED_NAME_CHARS) == strlen(name))
+                return add_placeholder_variable(name, elevel);
+            /* A special error message seems desirable here */
+            if (!skip_errors)
+                ereport(elevel,
+                        (errcode(ERRCODE_INVALID_NAME),
+                         errmsg("invalid configuration parameter name \"%s\"",
+                                name),
+                         errdetail("Configuration parameter names cannot contain \"=\" or \"-\".")));
+            return NULL;
+        }
     }

     /* Unknown name */
+    if (!skip_errors)
+        ereport(elevel,
+                (errcode(ERRCODE_UNDEFINED_OBJECT),
+                 errmsg("unrecognized configuration parameter \"%s\"",
+                        name)));
     return NULL;
 }

@@ -6354,7 +6384,7 @@ ReportChangedGUCOptions(void)
     {
         struct config_generic *record;

-        record = find_option("in_hot_standby", false, ERROR);
+        record = find_option("in_hot_standby", false, false, ERROR);
         Assert(record != NULL);
         record->status |= GUC_NEEDS_REPORT;
         report_needed = true;
@@ -7124,14 +7154,9 @@ set_config_option(const char *name, const char *value,
                 (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
                  errmsg("cannot set parameters during a parallel operation")));

-    record = find_option(name, true, elevel);
+    record = find_option(name, true, false, elevel);
     if (record == NULL)
-    {
-        ereport(elevel,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"", name)));
         return 0;
-    }

     /*
      * Check if the option can be set at this time. See guc.h for the precise
@@ -7853,10 +7878,10 @@ set_config_sourcefile(const char *name, char *sourcefile, int sourceline)
      */
     elevel = IsUnderPostmaster ? DEBUG3 : LOG;

-    record = find_option(name, true, elevel);
+    record = find_option(name, true, false, elevel);
     /* should not happen */
     if (record == NULL)
-        elog(ERROR, "unrecognized configuration parameter \"%s\"", name);
+        return;

     sourcefile = guc_strdup(elevel, sourcefile);
     if (record->sourcefile)
@@ -7905,16 +7930,9 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
     struct config_generic *record;
     static char buffer[256];

-    record = find_option(name, false, ERROR);
+    record = find_option(name, false, missing_ok, ERROR);
     if (record == NULL)
-    {
-        if (missing_ok)
-            return NULL;
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"",
-                        name)));
-    }
+        return NULL;
     if (restrict_privileged &&
         (record->flags & GUC_SUPERUSER_ONLY) &&
         !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
@@ -7961,11 +7979,8 @@ GetConfigOptionResetString(const char *name)
     struct config_generic *record;
     static char buffer[256];

-    record = find_option(name, false, ERROR);
-    if (record == NULL)
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"", name)));
+    record = find_option(name, false, false, ERROR);
+    Assert(record != NULL);
     if ((record->flags & GUC_SUPERUSER_ONLY) &&
         !is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_ALL_SETTINGS))
         ereport(ERROR,
@@ -8009,16 +8024,9 @@ GetConfigOptionFlags(const char *name, bool missing_ok)
 {
     struct config_generic *record;

-    record = find_option(name, false, WARNING);
+    record = find_option(name, false, missing_ok, ERROR);
     if (record == NULL)
-    {
-        if (missing_ok)
-            return 0;
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"",
-                        name)));
-    }
+        return 0;
     return record->flags;
 }

@@ -8050,7 +8058,7 @@ flatten_set_variable_args(const char *name, List *args)
      * Get flags for the variable; if it's not known, use default flags.
      * (Caller might throw error later, but not our business to do so here.)
      */
-    record = find_option(name, false, WARNING);
+    record = find_option(name, false, true, WARNING);
     if (record)
         flags = record->flags;
     else
@@ -8345,12 +8353,8 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
     {
         struct config_generic *record;

-        record = find_option(name, false, ERROR);
-        if (record == NULL)
-            ereport(ERROR,
-                    (errcode(ERRCODE_UNDEFINED_OBJECT),
-                     errmsg("unrecognized configuration parameter \"%s\"",
-                            name)));
+        record = find_option(name, false, false, ERROR);
+        Assert(record != NULL);

         /*
          * Don't allow parameters that can't be set in configuration files to
@@ -9366,19 +9370,12 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
 {
     struct config_generic *record;

-    record = find_option(name, false, ERROR);
+    record = find_option(name, false, missing_ok, ERROR);
     if (record == NULL)
     {
-        if (missing_ok)
-        {
-            if (varname)
-                *varname = NULL;
-            return NULL;
-        }
-
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"", name)));
+        if (varname)
+            *varname = NULL;
+        return NULL;
     }

     if ((record->flags & GUC_SUPERUSER_ONLY) &&
@@ -10230,7 +10227,7 @@ read_nondefault_variables(void)
         if ((varname = read_string_with_null(fp)) == NULL)
             break;

-        if ((record = find_option(varname, true, FATAL)) == NULL)
+        if ((record = find_option(varname, true, false, FATAL)) == NULL)
             elog(FATAL, "failed to locate variable \"%s\" in exec config params file", varname);

         if ((varvalue = read_string_with_null(fp)) == NULL)
@@ -10801,7 +10798,7 @@ GUCArrayAdd(ArrayType *array, const char *name, const char *value)
     (void) validate_option_array_item(name, value, false);

     /* normalize name (converts obsolete GUC names to modern spellings) */
-    record = find_option(name, false, WARNING);
+    record = find_option(name, false, true, WARNING);
     if (record)
         name = record->name;

@@ -10880,7 +10877,7 @@ GUCArrayDelete(ArrayType *array, const char *name)
     (void) validate_option_array_item(name, NULL, false);

     /* normalize name (converts obsolete GUC names to modern spellings) */
-    record = find_option(name, false, WARNING);
+    record = find_option(name, false, true, WARNING);
     if (record)
         name = record->name;

@@ -11027,7 +11024,7 @@ validate_option_array_item(const char *name, const char *value,
      * SUSET and user is superuser).
      *
      * name is not known, but exists or can be created as a placeholder (i.e.,
-     * it has a prefixed name).  We allow this case if you're a superuser,
+     * it has a valid custom name).  We allow this case if you're a superuser,
      * otherwise not.  Superusers are assumed to know what they're doing. We
      * can't allow it for other users, because when the placeholder is
      * resolved it might turn out to be a SUSET variable;
@@ -11036,16 +11033,11 @@ validate_option_array_item(const char *name, const char *value,
      * name is not known and can't be created as a placeholder.  Throw error,
      * unless skipIfNoPermissions is true, in which case return false.
      */
-    gconf = find_option(name, true, WARNING);
+    gconf = find_option(name, true, skipIfNoPermissions, ERROR);
     if (!gconf)
     {
         /* not known, failed to make a placeholder */
-        if (skipIfNoPermissions)
-            return false;
-        ereport(ERROR,
-                (errcode(ERRCODE_UNDEFINED_OBJECT),
-                 errmsg("unrecognized configuration parameter \"%s\"",
-                        name)));
+        return false;
     }

     if (gconf->flags & GUC_CUSTOM_PLACEHOLDER)
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 5004ee4177..e4bc5052ef 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -197,8 +197,16 @@ typedef enum
     GUC_ACTION_SAVE                /* function SET option, or temp assignment */
 } GucAction;

+/* Custom GUC names must contain this: */
 #define GUC_QUALIFIER_SEPARATOR '.'

+/*
+ * We disallow these characters in GUC names, as they would cause
+ * ParseLongOption() to do the wrong thing.  If you change this list,
+ * adjust the error text in find_option().
+ */
+#define GUC_DISALLOWED_NAME_CHARS "=-"
+
 /*
  * bit values in "flags" of a GUC variable
  */
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 811f80a097..63e2d31454 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -511,6 +511,29 @@ SET seq_page_cost TO 'NaN';
 ERROR:  invalid value for parameter "seq_page_cost": "NaN"
 SET vacuum_cost_delay TO '10s';
 ERROR:  10000 ms is outside the valid range for parameter "vacuum_cost_delay" (0 .. 100)
+SET no_such_variable TO 42;
+ERROR:  unrecognized configuration parameter "no_such_variable"
+-- Test "custom" GUCs created on the fly (which aren't really an
+-- intended feature, but many people use them).
+SET custom.my_guc = 42;
+SHOW custom.my_guc;
+ custom.my_guc
+---------------
+ 42
+(1 row)
+
+SET special."weird name" = 'foo';
+SHOW special."weird name";
+ special.weird name
+--------------------
+ foo
+(1 row)
+
+SET custom."bad-guc" = 42;  -- disallowed because -c cannot set it
+ERROR:  invalid configuration parameter name "custom.bad-guc"
+DETAIL:  Configuration parameter names cannot contain "=" or "-".
+SHOW custom."bad-guc";
+ERROR:  unrecognized configuration parameter "custom.bad-guc"
 --
 -- Test DISCARD TEMP
 --
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index 43dbba3775..c8f55a43be 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -147,6 +147,16 @@ SELECT '2006-08-13 12:34:56'::timestamptz;
 -- Test some simple error cases
 SET seq_page_cost TO 'NaN';
 SET vacuum_cost_delay TO '10s';
+SET no_such_variable TO 42;
+
+-- Test "custom" GUCs created on the fly (which aren't really an
+-- intended feature, but many people use them).
+SET custom.my_guc = 42;
+SHOW custom.my_guc;
+SET special."weird name" = 'foo';
+SHOW special."weird name";
+SET custom."bad-guc" = 42;  -- disallowed because -c cannot set it
+SHOW custom."bad-guc";

 --
 -- Test DISCARD TEMP

On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
> Now granting that the best answer is just to forbid these cases,
> there are still a couple of decisions about how extensive the
> prohibition ought to be:
> 
> * We could forbid these characters only when you try to actually
> put such a GUC into pg_db_role_setting, and otherwise allow them.
> That seems like a weird nonorthogonal choice though, so I'd
> rather just forbid them period.

Agreed.

> * A case could be made for tightening things up a lot more, and not
> allowing anything that doesn't look like an identifier.  I'm not
> pushing for that, as it seems more likely to break existing
> applications than the narrow restriction proposed here.  But I could
> live with it if people prefer that way.

I'd prefer that.  Characters like backslash, space, and double quote have
significant potential to reveal bugs, while having negligible application
beyond revealing bugs.

Noah Misch <noah@leadboat.com> writes:
> On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
>> * A case could be made for tightening things up a lot more, and not
>> allowing anything that doesn't look like an identifier.  I'm not
>> pushing for that, as it seems more likely to break existing
>> applications than the narrow restriction proposed here.  But I could
>> live with it if people prefer that way.

> I'd prefer that.  Characters like backslash, space, and double quote have
> significant potential to reveal bugs, while having negligible application
> beyond revealing bugs.

Any other opinions here?  I'm hesitant to make such a change on the
basis of just one vote.

            regards, tom lane

On Tue, Feb 9, 2021 at 6:02 PM Noah Misch <noah@leadboat.com> wrote:
> > * A case could be made for tightening things up a lot more, and not
> > allowing anything that doesn't look like an identifier.  I'm not
> > pushing for that, as it seems more likely to break existing
> > applications than the narrow restriction proposed here.  But I could
> > live with it if people prefer that way.
>
> I'd prefer that.  Characters like backslash, space, and double quote have
> significant potential to reveal bugs, while having negligible application
> beyond revealing bugs.

I'm not sure exactly what the rule should be here, but in general I
agree that a broader prohibition might be better. It's hard to
understand the rationale behind a system that doesn't allow
robert.max-workers as a GUC name, but does permit ro
b"ert.max^Hworkers.

+1 for not back-patching whatever we do here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On 2/11/21 1:32 PM, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
>> On Tue, Feb 09, 2021 at 05:34:37PM -0500, Tom Lane wrote:
>>> * A case could be made for tightening things up a lot more, and not
>>> allowing anything that doesn't look like an identifier.  I'm not
>>> pushing for that, as it seems more likely to break existing
>>> applications than the narrow restriction proposed here.  But I could
>>> live with it if people prefer that way.
>> I'd prefer that.  Characters like backslash, space, and double quote have
>> significant potential to reveal bugs, while having negligible application
>> beyond revealing bugs.
> Any other opinions here?  I'm hesitant to make such a change on the
> basis of just one vote.
>
>             



That might be a bit restrictive. I could at least see allowing '-' as
reasonable, and maybe ':'. Not sure about other punctuation characters.
OTOH I'd be surprised if the identifier restriction would burden a large
number of people.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Thu, Feb 11, 2021 at 02:50:13PM -0500, Robert Haas wrote:
> +1 for not back-patching whatever we do here.

+1.
--
Michael

I wrote:
> We can't allow '-', for the specific reason that it won't work as a -c
> argument (thanks to -c's translation of '-' to '_').  The whole point here
> is to prevent corner cases like that.  ':' would be all right, but I think
> it's a lot simpler to explain and a lot harder to break in future if we
> just say that the names have to be valid identifiers.

Hearing no further comments, I pushed the more restrictive version.

            regards, tom lane