From 3db826bd55cd1df0dd8c3d811f8e5b936d7ba1e4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 7 Apr 2021 11:22:22 -0400 Subject: [PATCH] Tighten up allowed names for custom GUC parameters. Formerly we were pretty lax about what a custom GUC's name could be; so long as it had at least one dot in it, we'd take it. However, corner cases such as dashes or equal signs in the name would cause various bits of functionality to misbehave. Rather than trying to make the world perfectly safe for that, let's just require that custom names look like "identifier.identifier", where "identifier" means something that scan.l would accept without double quotes. Along the way, this patch refactors things slightly in guc.c so that find_option() is responsible for reporting GUC-not-found cases, allowing removal of duplicative code from its callers. Per report from Hubert Depesz Lubaczewski. No back-patch, since the consequences of the problem don't seem to warrant changing behavior in stable branches. Discussion: https://postgr.es/m/951335.1612910077@sss.pgh.pa.us --- src/backend/utils/misc/guc-file.l | 4 +- src/backend/utils/misc/guc.c | 167 ++++++++++++++++++------------ src/test/regress/expected/guc.out | 21 ++++ src/test/regress/sql/guc.sql | 10 ++ 4 files changed, 132 insertions(+), 70 deletions(-) diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index 7885a169bb..9498bbea2f 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,7 @@ 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 (!valid_custom_variable_name(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 c9c9da85f3..1b007ca85c 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -5334,6 +5334,45 @@ add_guc_variable(struct config_generic *var, int elevel) return true; } +/* + * Decide whether a proposed custom variable name is allowed. + * + * It must be "identifier.identifier", where the rules for what is an + * identifier agree with scan.l. + */ +static bool +valid_custom_variable_name(const char *name) +{ + int num_sep = 0; + bool name_start = true; + + for (const char *p = name; *p; p++) + { + if (*p == GUC_QUALIFIER_SEPARATOR) + { + if (name_start) + return false; /* empty name component */ + num_sep++; + name_start = true; + } + else if (strchr("ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "abcdefghijklmnopqrstuvwxyz", *p) != NULL || + IS_HIGHBIT_SET(*p)) + { + /* okay as first or non-first character */ + name_start = false; + } + else if (!name_start && strchr("0123456789_$", *p) != NULL) + /* okay as non-first character */ ; + else + return false; + } + if (name_start) + return false; /* empty name component */ + /* OK if we had exactly one separator */ + return (num_sep == 1); +} + /* * Create and add a placeholder variable for a custom variable name. */ @@ -5381,12 +5420,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; @@ -5414,19 +5464,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. If it + * doesn't contain a separator, don't assume that it was meant to be a + * placeholder. */ if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL) - return add_placeholder_variable(name, elevel); + { + if (valid_custom_variable_name(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("Custom parameter names must be of the form \"identifier.identifier\"."))); + return NULL; + } } /* Unknown name */ + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("unrecognized configuration parameter \"%s\"", + name))); return NULL; } @@ -6444,7 +6513,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; @@ -7218,14 +7287,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 @@ -7947,10 +8011,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) @@ -7999,16 +8063,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(), ROLE_PG_READ_ALL_SETTINGS)) @@ -8055,11 +8112,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(), ROLE_PG_READ_ALL_SETTINGS)) ereport(ERROR, @@ -8103,16 +8157,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; } @@ -8144,7 +8191,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 @@ -8439,12 +8486,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 @@ -9460,19 +9503,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) && @@ -10318,7 +10354,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) @@ -11008,7 +11044,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; @@ -11087,7 +11123,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; @@ -11234,7 +11270,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; @@ -11243,16 +11279,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/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 811f80a097..c55871a972 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -511,6 +511,27 @@ 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 custom."bad-guc" = 42; -- disallowed because -c cannot set this name +ERROR: invalid configuration parameter name "custom.bad-guc" +DETAIL: Custom parameter names must be of the form "identifier.identifier". +SHOW custom."bad-guc"; +ERROR: unrecognized configuration parameter "custom.bad-guc" +SET special."weird name" = 'foo'; -- could be allowed, but we choose not to +ERROR: invalid configuration parameter name "special.weird name" +DETAIL: Custom parameter names must be of the form "identifier.identifier". +SHOW special."weird name"; +ERROR: unrecognized configuration parameter "special.weird name" -- -- Test DISCARD TEMP -- diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index 43dbba3775..3650188d9d 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 custom."bad-guc" = 42; -- disallowed because -c cannot set this name +SHOW custom."bad-guc"; +SET special."weird name" = 'foo'; -- could be allowed, but we choose not to +SHOW special."weird name"; -- -- Test DISCARD TEMP