From cb7928e05080e938ca21fdadcc21856750a73a5b Mon Sep 17 00:00:00 2001 From: Jacob Champion Date: Fri, 18 Sep 2026 10:48:08 -0700 Subject: [PATCH v2] Add a check_hook for output_plugin_libraries I omitted explicit syntax validation of output_plugin_libraries in 226e49cbed. [ALTER SYSTEM] SET does this implicitly due to the parser grammar, but a DBA could still accidentally put a bad value into postgresql.conf, and a superuser could do the same with connection options or set_config(). Logical replication would then fail later on. This is an annoying papercut for a security option to have, so reject these cases immediately in a check_hook. Tests by Hayato Kuroda. Co-authored-by: Hayato Kuroda Reported-by: Fujii Masao Reviewed-by: Hayato Kuroda Backpatch-through: 14 --- src/include/utils/guc_hooks.h | 2 + src/backend/replication/logical/logical.c | 46 ++++++++++++++++++----- src/backend/utils/misc/guc_parameters.dat | 1 + src/test/regress/expected/guc.out | 4 ++ src/test/regress/sql/guc.sql | 3 ++ 5 files changed, 46 insertions(+), 10 deletions(-) diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 06453a18c03..8e3c067f760 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -91,6 +91,8 @@ extern bool check_multixact_member_buffers(int *newval, void **extra, extern bool check_multixact_offset_buffers(int *newval, void **extra, GucSource source); extern bool check_notify_buffers(int *newval, void **extra, GucSource source); +extern bool check_output_plugin_libraries(char **newval, void **extra, + GucSource source); extern bool check_primary_slot_name(char **newval, void **extra, GucSource source); extern bool check_random_seed(double *newval, void **extra, GucSource source); diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index 98e5f1dd8f9..81e1452d2de 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -44,6 +44,7 @@ #include "storage/procarray.h" #include "utils/builtins.h" #include "utils/guc.h" +#include "utils/guc_hooks.h" #include "utils/injection_point.h" #include "utils/inval.h" #include "utils/memutils.h" @@ -109,6 +110,39 @@ static void update_progress_txn_cb_wrapper(ReorderBuffer *cache, static void LoadOutputPlugin(OutputPluginCallbacks *callbacks, const char *plugin); +/* check_hook: validate new output_plugin_libraries value */ +bool +check_output_plugin_libraries(char **newval, void **extra, GucSource source) +{ + char *copy; + List *components; + bool ok = true; + + copy = guc_strdup(LOG, *newval); + if (!copy) + return false; + + /* + * XXX SplitGUCList won't respect guc_malloc requirements, but this is + * consistent with other check_hook implementations... + */ + if (!SplitGUCList(copy, ',', &components)) + { + GUC_check_errdetail("List syntax is invalid."); + ok = false; + } + + /* + * Like with related GUC_LIST_QUOTE variables, we check only syntax here + * and not the existence of the plugins themselves. + */ + + list_free(components); + guc_free(copy); + + return ok; +} + /* * Make sure the current settings & environment are capable of doing logical * decoding. @@ -211,17 +245,9 @@ StartupDecodingContext(List *output_plugin_options, /* Need a modifiable copy */ rawstring = pstrdup(output_plugin_libraries_string); + /* The check_hook should have already handled syntax errors. */ if (!SplitGUCList(rawstring, ',', &elemlist)) - { - /* syntax error in list */ - ereport(LOG, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("invalid list syntax in parameter \"%s\"", - "output_plugin_libraries"))); - - list_free(elemlist); - elemlist = NIL; - } + elog(ERROR, "invalid output_plugin_libraries syntax after check_hook?"); foreach_ptr(char, allowed, elemlist) { diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index c57441f7d98..61c591314af 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2333,6 +2333,7 @@ flags => 'GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_SUPERUSER_ONLY', variable => 'output_plugin_libraries_string', boot_val => '"pgoutput, test_decoding"', + check_hook => 'check_output_plugin_libraries', }, { name => 'parallel_leader_participation', type => 'bool', context => 'PGC_USERSET', group => 'RESOURCES_WORKER_PROCESSES', diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 0c18fc94e31..d429e00985b 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -53,6 +53,10 @@ LINE 1: SET search_path = null, null; SET enable_seqscan = null; -- error ERROR: NULL is an invalid value for enable_seqscan RESET search_path; +-- Check syntax validation of output_plugin_libraries +SELECT set_config('output_plugin_libraries', 'pgoutput,', true); +ERROR: invalid value for parameter "output_plugin_libraries": "pgoutput," +DETAIL: List syntax is invalid. -- SET LOCAL has no effect outside of a transaction SET LOCAL vacuum_cost_delay TO 50; WARNING: SET LOCAL can only be used in transaction blocks diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index e78b4af3a3a..dd8fe84b7cc 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -21,6 +21,9 @@ SET search_path = null, null; -- syntax error SET enable_seqscan = null; -- error RESET search_path; +-- Check syntax validation of output_plugin_libraries +SELECT set_config('output_plugin_libraries', 'pgoutput,', true); + -- SET LOCAL has no effect outside of a transaction SET LOCAL vacuum_cost_delay TO 50; SHOW vacuum_cost_delay; -- 2.34.1