From 69560e81bfbb43a67269f54b564fe8ac5ae1b5c6 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Mon, 13 Dec 2021 08:42:38 -0600 Subject: [PATCH v4 1/3] warn when setting GUC to a nonextant library --- src/backend/utils/misc/guc.c | 103 +++++++++++++++++- .../unsafe_tests/expected/rolenames.out | 1 + .../worker_spi/expected/worker_spi.out | 2 + .../modules/worker_spi/sql/worker_spi.sql | 2 + src/test/regress/expected/rules.out | 9 ++ src/test/regress/sql/rules.sql | 1 + 6 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index f505413a7f9..c44b8ebbfd6 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -170,6 +170,12 @@ static bool call_enum_check_hook(struct config_enum *conf, int *newval, void **extra, GucSource source, int elevel); static bool check_log_destination(char **newval, void **extra, GucSource source); +static bool check_local_preload_libraries(char **newval, void **extra, + GucSource source); +static bool check_session_preload_libraries(char **newval, void **extra, + GucSource source); +static bool check_shared_preload_libraries(char **newval, void **extra, + GucSource source); static void assign_log_destination(const char *newval, void *extra); static bool check_wal_consistency_checking(char **newval, void **extra, @@ -4198,7 +4204,7 @@ static struct config_string ConfigureNamesString[] = }, &session_preload_libraries_string, "", - NULL, NULL, NULL + check_session_preload_libraries, NULL, NULL }, { @@ -4209,7 +4215,7 @@ static struct config_string ConfigureNamesString[] = }, &shared_preload_libraries_string, "", - NULL, NULL, NULL + check_shared_preload_libraries, NULL, NULL }, { @@ -4220,7 +4226,7 @@ static struct config_string ConfigureNamesString[] = }, &local_preload_libraries_string, "", - NULL, NULL, NULL + check_local_preload_libraries, NULL, NULL }, { @@ -12149,6 +12155,97 @@ check_max_worker_processes(int *newval, void **extra, GucSource source) return true; } +/* + * See also load_libraries() and internal_load_library(). + */ +static bool +check_preload_libraries(char **newval, void **extra, GucSource source, + bool restricted, bool is_session) +{ + char *rawstring; + List *elemlist; + ListCell *l; + + /* nothing to do if empty */ + if (newval == NULL || *newval[0] == '\0') + return true; + + /* + * Do not issue warnings while applying GUCs during startup. That would + * issue a FATAL error message, and the warnings would be redundant. + */ + if (!IsUnderPostmaster) + return true; + + /* Need a modifiable copy of string */ + rawstring = pstrdup(*newval); + + /* Parse string into list of filename paths */ + if (!SplitDirectoriesString(rawstring, ',', &elemlist)) + { + /* Should not happen ? */ + return false; + } + + foreach(l, elemlist) + { + /* Note that filename was already canonicalized */ + char *filename = (char *) lfirst(l); + char *expanded = NULL; + + /* If restricting, insert $libdir/plugins if not mentioned already */ + if (restricted && first_dir_separator(filename) == NULL) + { + expanded = psprintf("$libdir/plugins/%s", filename); + filename = expanded; + } + + /* + * stat()/access() only check that the library exists, not that it has + * the correct magic number or even a library. But error messages from + * dlopen() are not portable, so it'd be hard to report any problem + * other than "does not exist". + */ + if (access(filename, R_OK) == 0) // F_OK + continue; + + if (is_session) + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errhint("New sessions will fail with the existing configuration.")); + else + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errhint("The server will fail to start with the existing configuration." + " If it is is shut down, it will be necessary to manually edit the %s file to allow it to start.", + "postgresql.auto.conf")); + } + + list_free_deep(elemlist); + pfree(rawstring); + return true; +} + +static bool +check_shared_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, true, false); +} + +static bool +check_local_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, false, false); +} + +static bool +check_session_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, true, true); +} + static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source) { diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out index eb608fdc2ea..368b10558d7 100644 --- a/src/test/modules/unsafe_tests/expected/rolenames.out +++ b/src/test/modules/unsafe_tests/expected/rolenames.out @@ -1066,6 +1066,7 @@ GRANT pg_read_all_settings TO regress_role_haspriv; BEGIN; -- A GUC using GUC_SUPERUSER_ONLY is useful for negative tests. SET LOCAL session_preload_libraries TO 'path-to-preload-libraries'; +WARNING: could not access file "$libdir/plugins/path-to-preload-libraries" SET SESSION AUTHORIZATION regress_role_haspriv; -- passes with role member of pg_read_all_settings SHOW session_preload_libraries; diff --git a/src/test/modules/worker_spi/expected/worker_spi.out b/src/test/modules/worker_spi/expected/worker_spi.out index dc0a79bf759..5b275669bcf 100644 --- a/src/test/modules/worker_spi/expected/worker_spi.out +++ b/src/test/modules/worker_spi/expected/worker_spi.out @@ -28,6 +28,8 @@ SELECT pg_reload_conf(); t (1 row) +-- reconnect to avoid unstable test result due to asynchronous signal +\c -- wait until the worker has processed the tuple we just inserted DO $$ DECLARE diff --git a/src/test/modules/worker_spi/sql/worker_spi.sql b/src/test/modules/worker_spi/sql/worker_spi.sql index 4683523b29d..4ed5370d456 100644 --- a/src/test/modules/worker_spi/sql/worker_spi.sql +++ b/src/test/modules/worker_spi/sql/worker_spi.sql @@ -18,6 +18,8 @@ END $$; INSERT INTO schema4.counted VALUES ('total', 0), ('delta', 1); SELECT pg_reload_conf(); +-- reconnect to avoid unstable test result due to asynchronous signal +\c -- wait until the worker has processed the tuple we just inserted DO $$ DECLARE diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out index 1420288d67b..f3d33dda95e 100644 --- a/src/test/regress/expected/rules.out +++ b/src/test/regress/expected/rules.out @@ -3362,6 +3362,7 @@ drop table hats; drop table hat_data; -- test for pg_get_functiondef properly regurgitating SET parameters -- Note that the function is kept around to stress pg_dump. +SET check_function_bodies=no; CREATE FUNCTION func_with_set_params() RETURNS integer AS 'select 1;' LANGUAGE SQL @@ -3371,6 +3372,14 @@ CREATE FUNCTION func_with_set_params() RETURNS integer SET datestyle to iso, mdy SET local_preload_libraries TO "Mixed/Case", 'c:/''a"/path', '', '0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789' IMMUTABLE STRICT; +WARNING: could not access file "Mixed/Case" +HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start. +WARNING: could not access file "c:/'a"/path" +HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start. +WARNING: could not access file "" +HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start. +WARNING: could not access file "0123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789" +HINT: The server will fail to start with the existing configuration. If it is is shut down, it will be necessary to manually edit the postgresql.auto.conf file to allow it to start. SELECT pg_get_functiondef('func_with_set_params()'::regprocedure); pg_get_functiondef -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql index 8bdab6dec30..6308e5d27fd 100644 --- a/src/test/regress/sql/rules.sql +++ b/src/test/regress/sql/rules.sql @@ -1198,6 +1198,7 @@ drop table hat_data; -- test for pg_get_functiondef properly regurgitating SET parameters -- Note that the function is kept around to stress pg_dump. +SET check_function_bodies=no; CREATE FUNCTION func_with_set_params() RETURNS integer AS 'select 1;' LANGUAGE SQL -- 2.17.1