From 88103567cb8fa5be46dc9fac3e3b8774951a2be7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 21 Feb 2022 14:10:15 -0500 Subject: [PATCH] Disallow setting bogus GUCs within an extension's reserved namespace. Commit 75d22069e tried to throw a warning for setting a custom GUC whose prefix belongs to a previously-loaded extension, if there is no such GUC defined by the extension. But that caused unstable behavior with parallel workers, because workers don't necessarily load extensions and GUCs in the same order their leader did. To make that work safely, we have to completely disallow the case. We now actually remove any such GUCs at the time of initial extension load, and then throw an error not just a warning if you try to add one later. While this might create a compatibility issue for a few people, the improvement in error-detection capability seems worth it; it's hard to believe that there's any good use-case for choosing such GUC names. This also un-reverts 5609cc01c (Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved()), since that function's old name is now even more of a misnomer. Florin Irion and Tom Lane Discussion: https://postgr.es/m/1902182.1640711215@sss.pgh.pa.us --- contrib/auth_delay/auth_delay.c | 2 +- contrib/auto_explain/auto_explain.c | 2 +- contrib/basic_archive/basic_archive.c | 2 +- contrib/pg_prewarm/autoprewarm.c | 2 +- .../pg_stat_statements/pg_stat_statements.c | 2 +- contrib/pg_trgm/trgm_op.c | 2 +- contrib/postgres_fdw/option.c | 2 +- contrib/sepgsql/hooks.c | 2 +- src/backend/utils/misc/guc.c | 79 +++++++++++++++---- src/include/utils/guc.h | 5 +- src/pl/plperl/plperl.c | 2 +- src/pl/plpgsql/src/pl_handler.c | 2 +- src/pl/tcl/pltcl.c | 4 +- .../modules/delay_execution/delay_execution.c | 2 +- .../ssl_passphrase_func.c | 2 +- src/test/modules/worker_spi/worker_spi.c | 2 +- src/test/regress/expected/guc.out | 11 +++ src/test/regress/sql/guc.sql | 7 ++ 18 files changed, 101 insertions(+), 31 deletions(-) diff --git a/contrib/auth_delay/auth_delay.c b/contrib/auth_delay/auth_delay.c index 38f4276db39..6b94d653ea4 100644 --- a/contrib/auth_delay/auth_delay.c +++ b/contrib/auth_delay/auth_delay.c @@ -68,7 +68,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("auth_delay"); + MarkGUCPrefixReserved("auth_delay"); /* Install Hooks */ original_client_auth_hook = ClientAuthentication_hook; diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index 3e09abaecac..d3029f85efe 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -231,7 +231,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("auto_explain"); + MarkGUCPrefixReserved("auto_explain"); /* Install hooks. */ prev_ExecutorStart = ExecutorStart_hook; diff --git a/contrib/basic_archive/basic_archive.c b/contrib/basic_archive/basic_archive.c index 16ddddccbbc..e7efbfb9c34 100644 --- a/contrib/basic_archive/basic_archive.c +++ b/contrib/basic_archive/basic_archive.c @@ -69,7 +69,7 @@ _PG_init(void) 0, check_archive_directory, NULL, NULL); - EmitWarningsOnPlaceholders("basic_archive"); + MarkGUCPrefixReserved("basic_archive"); basic_archive_context = AllocSetContextCreate(TopMemoryContext, "basic_archive", diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 1d4d74b171f..45e012a63a5 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -137,7 +137,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("pg_prewarm"); + MarkGUCPrefixReserved("pg_prewarm"); RequestAddinShmemSpace(MAXALIGN(sizeof(AutoPrewarmSharedState))); diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 9d7d0812ac3..38d92a89cc4 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -437,7 +437,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("pg_stat_statements"); + MarkGUCPrefixReserved("pg_stat_statements"); /* * Request additional shared resources. (These are no-ops if we're not in diff --git a/contrib/pg_trgm/trgm_op.c b/contrib/pg_trgm/trgm_op.c index 0407c7dd644..e9b7981619f 100644 --- a/contrib/pg_trgm/trgm_op.c +++ b/contrib/pg_trgm/trgm_op.c @@ -101,7 +101,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("pg_trgm"); + MarkGUCPrefixReserved("pg_trgm"); } /* diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index af38e956e70..2c6b2894b96 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -538,5 +538,5 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("postgres_fdw"); + MarkGUCPrefixReserved("postgres_fdw"); } diff --git a/contrib/sepgsql/hooks.c b/contrib/sepgsql/hooks.c index d71c802106a..97e61b8043f 100644 --- a/contrib/sepgsql/hooks.c +++ b/contrib/sepgsql/hooks.c @@ -455,7 +455,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("sepgsql"); + MarkGUCPrefixReserved("sepgsql"); /* Initialize userspace access vector cache */ sepgsql_avc_init(); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 01f373815e0..eaa4bf2c304 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -150,6 +150,8 @@ extern bool optimize_bounded_sort; static int GUC_check_errcode_value; +static List *reserved_class_prefix = NIL; + /* global variables for check hook support */ char *GUC_check_errmsg_string; char *GUC_check_errdetail_string; @@ -5590,18 +5592,44 @@ find_option(const char *name, bool create_placeholders, bool skip_errors, * doesn't contain a separator, don't assume that it was meant to be a * placeholder. */ - if (strchr(name, GUC_QUALIFIER_SEPARATOR) != NULL) + const char *sep = strchr(name, GUC_QUALIFIER_SEPARATOR); + + if (sep != NULL) { - 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 two or more simple identifiers separated by dots."))); - return 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 */ + return add_placeholder_variable(name, elevel); } } @@ -9355,15 +9383,26 @@ DefineCustomEnumVariable(const char *name, } /* + * Mark the given GUC prefix as "reserved". + * + * This deletes any existing placeholders matching the prefix, + * and then prevents new ones from being created. * Extensions should call this after they've defined all of their custom * GUCs, to help catch misspelled config-file entries. */ void -EmitWarningsOnPlaceholders(const char *className) +MarkGUCPrefixReserved(const char *className) { int classLen = strlen(className); int i; + MemoryContext oldcontext; + /* + * Check for existing placeholders. We must actually remove invalid + * placeholders, else future parallel worker startups will fail. (We + * don't bother trying to free associated memory, since this shouldn't + * happen often.) + */ for (i = 0; i < num_guc_variables; i++) { struct config_generic *var = guc_variables[i]; @@ -9373,11 +9412,21 @@ EmitWarningsOnPlaceholders(const char *className) var->name[classLen] == GUC_QUALIFIER_SEPARATOR) { ereport(WARNING, - (errcode(ERRCODE_UNDEFINED_OBJECT), - errmsg("unrecognized configuration parameter \"%s\"", - var->name))); + (errcode(ERRCODE_INVALID_NAME), + errmsg("invalid configuration parameter name \"%s\", removing it", + var->name), + errdetail("\"%s\" is now a reserved prefix.", + className))); + num_guc_variables--; + memmove(&guc_variables[i], &guc_variables[i + 1], + (num_guc_variables - i) * sizeof(struct config_generic *)); } } + + /* And remember the name so we can prevent future mistakes. */ + oldcontext = MemoryContextSwitchTo(TopMemoryContext); + reserved_class_prefix = lappend(reserved_class_prefix, pstrdup(className)); + MemoryContextSwitchTo(oldcontext); } diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index f1bfe79feb8..ea774968f07 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -354,7 +354,10 @@ extern void DefineCustomEnumVariable(const char *name, GucEnumAssignHook assign_hook, GucShowHook show_hook); -extern void EmitWarningsOnPlaceholders(const char *className); +extern void MarkGUCPrefixReserved(const char *className); + +/* old name for MarkGUCPrefixReserved, for backwards compatibility: */ +#define EmitWarningsOnPlaceholders(className) MarkGUCPrefixReserved(className) extern const char *GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged); diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c index 3f785b1e8d5..b5879c29471 100644 --- a/src/pl/plperl/plperl.c +++ b/src/pl/plperl/plperl.c @@ -455,7 +455,7 @@ _PG_init(void) PGC_SUSET, 0, NULL, NULL, NULL); - EmitWarningsOnPlaceholders("plperl"); + MarkGUCPrefixReserved("plperl"); /* * Create hash tables. diff --git a/src/pl/plpgsql/src/pl_handler.c b/src/pl/plpgsql/src/pl_handler.c index b4b85092806..190d286f1c8 100644 --- a/src/pl/plpgsql/src/pl_handler.c +++ b/src/pl/plpgsql/src/pl_handler.c @@ -197,7 +197,7 @@ _PG_init(void) plpgsql_extra_errors_assign_hook, NULL); - EmitWarningsOnPlaceholders("plpgsql"); + MarkGUCPrefixReserved("plpgsql"); plpgsql_HashTableInit(); RegisterXactCallback(plpgsql_xact_cb, NULL); diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c index 7c045f45607..ab759833db1 100644 --- a/src/pl/tcl/pltcl.c +++ b/src/pl/tcl/pltcl.c @@ -474,8 +474,8 @@ _PG_init(void) PGC_SUSET, 0, NULL, NULL, NULL); - EmitWarningsOnPlaceholders("pltcl"); - EmitWarningsOnPlaceholders("pltclu"); + MarkGUCPrefixReserved("pltcl"); + MarkGUCPrefixReserved("pltclu"); pltcl_pm_init_done = true; } diff --git a/src/test/modules/delay_execution/delay_execution.c b/src/test/modules/delay_execution/delay_execution.c index ad50383bf8a..cf34e8c2d7d 100644 --- a/src/test/modules/delay_execution/delay_execution.c +++ b/src/test/modules/delay_execution/delay_execution.c @@ -91,7 +91,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("delay_execution"); + MarkGUCPrefixReserved("delay_execution"); /* Install our hook */ prev_planner_hook = planner_hook; diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c index 3ba33e501c3..7c469fd57e8 100644 --- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c +++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c @@ -49,7 +49,7 @@ _PG_init(void) NULL, NULL); - EmitWarningsOnPlaceholders("ssl_passphrase"); + MarkGUCPrefixReserved("ssl_passphrase"); if (ssl_passphrase) openssl_tls_init_hook = set_rot13; diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index 05ced63780e..48829df29c3 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -322,7 +322,7 @@ _PG_init(void) 0, NULL, NULL, NULL); - EmitWarningsOnPlaceholders("worker_spi"); + MarkGUCPrefixReserved("worker_spi"); /* set up common data for all our workers */ memset(&worker, 0, sizeof(worker)); diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 75b6bfbf116..3de6404ba5b 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -548,6 +548,17 @@ ERROR: invalid configuration parameter name "special.weird name" DETAIL: Custom parameter names must be two or more simple identifiers separated by dots. SHOW special."weird name"; ERROR: unrecognized configuration parameter "special.weird name" +-- Check what happens when you try to set a "custom" GUC within the +-- namespace of an extension. +SET plpgsql.extra_foo_warnings = true; -- allowed if plpgsql is not loaded yet +LOAD 'plpgsql'; -- this will throw a warning and delete the variable +WARNING: invalid configuration parameter name "plpgsql.extra_foo_warnings", removing it +DETAIL: "plpgsql" is now a reserved prefix. +SET plpgsql.extra_foo_warnings = true; -- now, it's an error +ERROR: invalid configuration parameter name "plpgsql.extra_foo_warnings" +DETAIL: "plpgsql" is a reserved prefix. +SHOW plpgsql.extra_foo_warnings; +ERROR: unrecognized configuration parameter "plpgsql.extra_foo_warnings" -- -- Test DISCARD TEMP -- diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index 3e2819449c2..d5db101e486 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -163,6 +163,13 @@ SHOW custom."bad-guc"; SET special."weird name" = 'foo'; -- could be allowed, but we choose not to SHOW special."weird name"; +-- Check what happens when you try to set a "custom" GUC within the +-- namespace of an extension. +SET plpgsql.extra_foo_warnings = true; -- allowed if plpgsql is not loaded yet +LOAD 'plpgsql'; -- this will throw a warning and delete the variable +SET plpgsql.extra_foo_warnings = true; -- now, it's an error +SHOW plpgsql.extra_foo_warnings; + -- -- Test DISCARD TEMP -- -- 2.39.5