diff --git a/src/backend/catalog/pg_parameter_acl.c b/src/backend/catalog/pg_parameter_acl.c index 073392e2c4..f4bc10bafe 100644 --- a/src/backend/catalog/pg_parameter_acl.c +++ b/src/backend/catalog/pg_parameter_acl.c @@ -82,11 +82,7 @@ ParameterAclCreate(const char *parameter) * To prevent cluttering pg_parameter_acl with useless entries, insist * that the name be valid. */ - if (!check_GUC_name_for_parameter_acl(parameter)) - ereport(ERROR, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid parameter name \"%s\"", - parameter))); + check_GUC_name_for_parameter_acl(parameter); /* Convert name to the form it should have in pg_parameter_acl. */ parname = convert_GUC_name_for_parameter_acl(parameter); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index c25c697a06..e1ea5561d7 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -250,6 +250,8 @@ static void write_auto_conf_file(int fd, const char *filename, ConfigVariable *h static void replace_auto_config_value(ConfigVariable **head_p, ConfigVariable **tail_p, const char *name, const char *value); static bool valid_custom_variable_name(const char *name); +static bool assignable_custom_variable_name(const char *name, bool skip_errors, + int elevel); static void do_serialize(char **destptr, Size *maxbytes, const char *fmt,...) pg_attribute_printf(3, 4); static bool call_bool_check_hook(struct config_bool *conf, bool *newval, @@ -1063,7 +1065,7 @@ add_guc_variable(struct config_generic *var, int elevel) * * It must be two or more identifiers separated by dots, where the rules * for what is an identifier agree with scan.l. (If you change this rule, - * adjust the errdetail in find_option().) + * adjust the errdetail in assignable_custom_variable_name().) */ static bool valid_custom_variable_name(const char *name) @@ -1098,6 +1100,71 @@ valid_custom_variable_name(const char *name) return saw_sep; } +/* + * Decide whether an unrecognized variable name is allowed to be SET. + * + * It must pass the syntactic rules of valid_custom_variable_name(), + * and it must not be in any namespace already reserved by an extension. + * (We make this separate from valid_custom_variable_name() because we don't + * apply the reserved-namespace test when reading configuration files.) + * + * If valid, return true. Otherwise, return false if skip_errors is true, + * else throw a suitable error at the specified elevel (and return false + * if that's less than ERROR). + */ +static bool +assignable_custom_variable_name(const char *name, bool skip_errors, int elevel) +{ + /* If there's no separator, it can't be a custom variable */ + const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR); + + if (sep != NULL) + { + size_t classLen = sep - name; + ListCell *lc; + + /* The name must be syntactically acceptable ... */ + if (!valid_custom_variable_name(name)) + { + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid configuration parameter name \"%s\"", + name), + errdetail("Custom parameter names must be two or more simple identifiers separated by dots."))); + return false; + } + /* ... and it must not match any previously-reserved prefix */ + foreach(lc, reserved_class_prefix) + { + const char *rcprefix = lfirst(lc); + + if (strlen(rcprefix) == classLen && + strncmp(name, rcprefix, classLen) == 0) + { + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid configuration parameter name \"%s\"", + name), + errdetail("\"%s\" is a reserved prefix.", + rcprefix))); + return false; + } + } + /* OK to create it */ + return true; + } + + /* Unrecognized single-part name */ + if (!skip_errors) + ereport(elevel, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("unrecognized configuration parameter \"%s\"", + name))); + return false; +} + /* * Create and add a placeholder variable for a custom variable name. */ @@ -1191,52 +1258,15 @@ find_option(const char *name, bool create_placeholders, bool skip_errors, if (create_placeholders) { /* - * 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. + * Check if the name is valid, and if so, add a placeholder. */ - const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR); - - if (sep != NULL) - { - size_t classLen = sep - name; - ListCell *lc; - - /* The name must be syntactically acceptable ... */ - if (!valid_custom_variable_name(name)) - { - if (!skip_errors) - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid configuration parameter name \"%s\"", - name), - errdetail("Custom parameter names must be two or more simple identifiers separated by dots."))); - return NULL; - } - /* ... and it must not match any previously-reserved prefix */ - foreach(lc, reserved_class_prefix) - { - const char *rcprefix = lfirst(lc); - - if (strlen(rcprefix) == classLen && - strncmp(name, rcprefix, classLen) == 0) - { - if (!skip_errors) - ereport(elevel, - (errcode(ERRCODE_INVALID_NAME), - errmsg("invalid configuration parameter name \"%s\"", - name), - errdetail("\"%s\" is a reserved prefix.", - rcprefix))); - return NULL; - } - } - /* OK, create it */ + if (assignable_custom_variable_name(name, skip_errors, elevel)) return add_placeholder_variable(name, elevel); - } + else + return NULL; /* error message, if any, already emitted */ } - /* Unknown name */ + /* Unknown name and we're not supposed to make a placeholder */ if (!skip_errors) ereport(elevel, (errcode(ERRCODE_UNDEFINED_OBJECT), @@ -1369,18 +1399,16 @@ convert_GUC_name_for_parameter_acl(const char *name) /* * Check whether we should allow creation of a pg_parameter_acl entry * for the given name. (This can be applied either before or after - * canonicalizing it.) + * canonicalizing it.) Throws error if not. */ -bool +void check_GUC_name_for_parameter_acl(const char *name) { /* OK if the GUC exists. */ - if (find_option(name, false, true, DEBUG1) != NULL) - return true; + if (find_option(name, false, true, DEBUG5) != NULL) + return; /* Otherwise, it'd better be a valid custom GUC name. */ - if (valid_custom_variable_name(name)) - return true; - return false; + (void) assignable_custom_variable_name(name, false, ERROR); } /* @@ -4515,9 +4543,10 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) { struct config_generic *record; - record = find_option(name, false, false, ERROR); - Assert(record != NULL); - + /* We don't want to create a placeholder if there's not one already */ + record = find_option(name, false, true, DEBUG5); + if (record != NULL) + { /* * Don't allow parameters that can't be set in configuration files to * be set in PG_AUTOCONF_FILENAME file. @@ -4561,6 +4590,18 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) (errcode(ERRCODE_INVALID_PARAMETER_VALUE), errmsg("parameter value for ALTER SYSTEM must not contain a newline"))); } + } + else + { + /* + * Variable not known; check we'd be allowed to create it. (We + * cannot validate the value, but that's fine. A non-core GUC in + * the config file cannot cause postmaster start to fail, so we + * don't have to be too tense about possibly installing a bad + * value.) + */ + (void) assignable_custom_variable_name(name, false, ERROR); + } } /* diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index e89083ee0e..902e57c0ca 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -363,7 +363,7 @@ extern const char *GetConfigOptionResetString(const char *name); extern int GetConfigOptionFlags(const char *name, bool missing_ok); extern void ProcessConfigFile(GucContext context); extern char *convert_GUC_name_for_parameter_acl(const char *name); -extern bool check_GUC_name_for_parameter_acl(const char *name); +extern void check_GUC_name_for_parameter_acl(const char *name); extern void InitializeGUCOptions(void); extern bool SelectConfigFiles(const char *userDoption, const char *progname); extern void ResetAllOptions(void); diff --git a/src/test/modules/unsafe_tests/expected/guc_privs.out b/src/test/modules/unsafe_tests/expected/guc_privs.out index f43a1da214..6c0ad89834 100644 --- a/src/test/modules/unsafe_tests/expected/guc_privs.out +++ b/src/test/modules/unsafe_tests/expected/guc_privs.out @@ -220,9 +220,31 @@ SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such'; ---------- (0 rows) +-- Superuser should be able to ALTER SYSTEM SET a non-existent custom GUC. +ALTER SYSTEM SET none.such = 'whiz bang'; +-- None of the above should have created a placeholder GUC for none.such. +SHOW none.such; -- error +ERROR: unrecognized configuration parameter "none.such" +-- However, if we reload ... +SELECT pg_reload_conf(); + pg_reload_conf +---------------- + t +(1 row) + +-- and start a new session to avoid race condition ... +\c - +SET SESSION AUTHORIZATION regress_admin; +-- then it should be there. +SHOW none.such; + none.such +----------- + whiz bang +(1 row) + -- Can't grant on a non-existent core GUC. GRANT ALL ON PARAMETER no_such_guc TO regress_host_resource_admin; -- fail -ERROR: invalid parameter name "no_such_guc" +ERROR: unrecognized configuration parameter "no_such_guc" -- Initially there are no privileges and no catalog entry for this GUC. SELECT has_parameter_privilege('regress_host_resource_admin', 'enable_material', 'SET'); has_parameter_privilege @@ -459,6 +481,8 @@ SELECT set_config ('temp_buffers', '8192', false); -- ok ALTER SYSTEM RESET autovacuum_work_mem; -- ok, privileges have been granted ALTER SYSTEM RESET ALL; -- fail, insufficient privileges ERROR: permission denied to perform ALTER SYSTEM RESET ALL +ALTER SYSTEM SET none.such2 = 'whiz bang'; -- fail, not superuser +ERROR: permission denied to set parameter "none.such2" ALTER ROLE regress_host_resource_admin SET lc_messages = 'POSIX'; -- fail ERROR: permission denied to set parameter "lc_messages" ALTER ROLE regress_host_resource_admin SET max_stack_depth = '1MB'; -- ok diff --git a/src/test/modules/unsafe_tests/sql/guc_privs.sql b/src/test/modules/unsafe_tests/sql/guc_privs.sql index 7a4fb24b9d..9bcbbfa904 100644 --- a/src/test/modules/unsafe_tests/sql/guc_privs.sql +++ b/src/test/modules/unsafe_tests/sql/guc_privs.sql @@ -98,6 +98,19 @@ GRANT ALL ON PARAMETER none.such TO regress_host_resource_admin; SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such'; REVOKE ALL ON PARAMETER "None.Such" FROM regress_host_resource_admin; SELECT 1 FROM pg_parameter_acl WHERE parname = 'none.such'; + +-- Superuser should be able to ALTER SYSTEM SET a non-existent custom GUC. +ALTER SYSTEM SET none.such = 'whiz bang'; +-- None of the above should have created a placeholder GUC for none.such. +SHOW none.such; -- error +-- However, if we reload ... +SELECT pg_reload_conf(); +-- and start a new session to avoid race condition ... +\c - +SET SESSION AUTHORIZATION regress_admin; +-- then it should be there. +SHOW none.such; + -- Can't grant on a non-existent core GUC. GRANT ALL ON PARAMETER no_such_guc TO regress_host_resource_admin; -- fail @@ -190,6 +203,7 @@ ALTER SYSTEM RESET lc_messages; -- fail, insufficient privileges SELECT set_config ('temp_buffers', '8192', false); -- ok ALTER SYSTEM RESET autovacuum_work_mem; -- ok, privileges have been granted ALTER SYSTEM RESET ALL; -- fail, insufficient privileges +ALTER SYSTEM SET none.such2 = 'whiz bang'; -- fail, not superuser ALTER ROLE regress_host_resource_admin SET lc_messages = 'POSIX'; -- fail ALTER ROLE regress_host_resource_admin SET max_stack_depth = '1MB'; -- ok SELECT setconfig FROM pg_db_role_setting