From 47eb2ab393e050ac98a6197a8302c83c23ca0df3 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Mon, 13 Dec 2021 08:42:38 -0600 Subject: [PATCH 2/4] warn when setting GUC to a nonextant library Note that warnings shouldn't be issued during startup (only fatals): $ ./tmp_install/usr/local/pgsql/bin/postgres -D ./testrun/regress/regress/tmp_check/data -c shared_preload_libraries=ab 2023-07-06 14:47:03.817 CDT postmaster[2608106] FATAL: could not access file "ab": No existe el archivo o el directorio 2023-07-06 14:47:03.817 CDT postmaster[2608106] CONTEXT: while loading shared libraries for setting "shared_preload_libraries" --- src/backend/commands/variable.c | 101 ++++++++++++++++++ src/backend/utils/fmgr/dfmgr.c | 3 +- src/backend/utils/misc/guc.c | 6 ++ src/backend/utils/misc/guc_tables.c | 6 +- src/include/fmgr.h | 1 + src/include/utils/guc_hooks.h | 7 ++ .../unsafe_tests/expected/rolenames.out | 19 ++++ .../modules/unsafe_tests/sql/rolenames.sql | 13 +++ 8 files changed, 151 insertions(+), 5 deletions(-) diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index c9421b17952..0714b4421dc 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -40,6 +40,7 @@ #include "utils/timestamp.h" #include "utils/tzparser.h" #include "utils/varlena.h" +#include /* * DATESTYLE @@ -1234,3 +1235,103 @@ check_ssl(bool *newval, void **extra, GucSource source) #endif return true; } + + +/* + * See also load_libraries() and internal_load_library(). + */ +static bool +check_preload_libraries(char **newval, void **extra, GucSource source, + bool restricted) +{ + char *rawstring; + List *elemlist; + ListCell *l; + + /* nothing to do if empty */ + if (newval == NULL || *newval[0] == '\0') + return true; + + /* + * Issue warnings only during an interactive SET from ALTER + * ROLE/DATABASE/SYSTEM, and not while applying GUCs during startup, + * when relevant errors would be FATAL, and the warnings would be redundant. + * + * IsUnderPostmaster is a hack to allow warning under ALTER SYSTEM but + * not during startup. Both of which use PGC_S_FILE. + */ + 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; + } + + filename = expand_dynamic_library_name(filename); + + /* + * 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) + continue; + + if (source == PGC_S_FILE) + /* ALTER SYSTEM */ + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errdetail("The server might fail to start with this setting."), + errhint("If the server is shut down, it will be necessary to manually edit the %s file to allow it to start again.", + "postgresql.auto.conf")); + else if (source == PGC_S_TEST) + /* ALTER ROLE/DATABASE */ + ereport(WARNING, + errcode_for_file_access(), + errmsg("could not access file \"%s\"", filename), + errdetail("New sessions will fail to connect with the new setting.")); + } + + list_free_deep(elemlist); + pfree(rawstring); + return true; +} + +bool +check_shared_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, false); +} + +bool +check_local_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, true); +} + +bool +check_session_preload_libraries(char **newval, void **extra, GucSource source) +{ + return check_preload_libraries(newval, extra, source, false); +} diff --git a/src/backend/utils/fmgr/dfmgr.c b/src/backend/utils/fmgr/dfmgr.c index 092004dcf3b..b03017f6275 100644 --- a/src/backend/utils/fmgr/dfmgr.c +++ b/src/backend/utils/fmgr/dfmgr.c @@ -79,7 +79,6 @@ char *Dynamic_library_path; static void *internal_load_library(const char *libname); static void incompatible_module_error(const char *libname, const Pg_magic_struct *module_magic_data) pg_attribute_noreturn(); -static char *expand_dynamic_library_name(const char *name); static void check_restricted_library_name(const char *name); static char *substitute_libpath_macro(const char *name); static char *find_in_dynamic_libpath(const char *basename); @@ -410,7 +409,7 @@ incompatible_module_error(const char *libname, * * The result will always be freshly palloc'd. */ -static char * +char * expand_dynamic_library_name(const char *name) { bool have_slash; diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index e2734475189..4cd40ca9678 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4654,6 +4654,12 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt) union config_var_val newval; void *newextra = NULL; + /* + * PGC_S_FILE is used even though we should use PGC_S_TEST, + * since various GUC validation functions use TEST to give a + * NOTICE rather than an ERROR, and we don't want to allow + * setting values that would always return an error later. + */ if (!parse_and_validate_value(record, name, value, PGC_S_FILE, ERROR, &newval, &newextra)) diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c index 630ed0f1629..6c424be4c47 100644 --- a/src/backend/utils/misc/guc_tables.c +++ b/src/backend/utils/misc/guc_tables.c @@ -4237,7 +4237,7 @@ struct config_string ConfigureNamesString[] = }, &session_preload_libraries_string, "", - NULL, NULL, NULL + check_session_preload_libraries, NULL, NULL }, { @@ -4248,7 +4248,7 @@ struct config_string ConfigureNamesString[] = }, &shared_preload_libraries_string, "", - NULL, NULL, NULL + check_shared_preload_libraries, NULL, NULL }, { @@ -4259,7 +4259,7 @@ struct config_string ConfigureNamesString[] = }, &local_preload_libraries_string, "", - NULL, NULL, NULL + check_local_preload_libraries, NULL, NULL }, { diff --git a/src/include/fmgr.h b/src/include/fmgr.h index ccb4070a251..c6319782a99 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -749,6 +749,7 @@ extern void **find_rendezvous_variable(const char *varName); extern Size EstimateLibraryStateSpace(void); extern void SerializeLibraryState(Size maxsize, char *start_address); extern void RestoreLibraryState(char *start_address); +char *expand_dynamic_library_name(const char *name); /* * Support for aggregate functions diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 153c652c93e..a11c5488041 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -172,4 +172,11 @@ extern bool check_synchronized_standby_slots(char **newval, void **extra, GucSource source); extern void assign_synchronized_standby_slots(const char *newval, void *extra); +extern bool check_local_preload_libraries(char **newval, void **extra, + GucSource source); +extern bool check_session_preload_libraries(char **newval, void **extra, + GucSource source); +extern bool check_shared_preload_libraries(char **newval, void **extra, + GucSource source); + #endif /* GUC_HOOKS_H */ diff --git a/src/test/modules/unsafe_tests/expected/rolenames.out b/src/test/modules/unsafe_tests/expected/rolenames.out index 61396b2a805..de707a4803e 100644 --- a/src/test/modules/unsafe_tests/expected/rolenames.out +++ b/src/test/modules/unsafe_tests/expected/rolenames.out @@ -1083,6 +1083,25 @@ RESET SESSION AUTHORIZATION; ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK; REVOKE pg_read_all_settings FROM regress_role_haspriv; +-- test some warnings +ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist"; +WARNING: could not access file "DoesNotExist" +DETAIL: New sessions will fail to connect with the new setting. +ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist"; +WARNING: could not access file "$libdir/plugins/DoesNotExist" +DETAIL: New sessions will fail to connect with the new setting. +CREATE DATABASE regression_nosuch_db; +ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist"; +WARNING: could not access file "DoesNotExist" +DETAIL: New sessions will fail to connect with the new setting. +ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist"; +WARNING: could not access file "$libdir/plugins/DoesNotExist" +DETAIL: New sessions will fail to connect with the new setting. +DROP DATABASE regression_nosuch_db; +-- SET doesn't do anything, but should not warn, either +SET session_preload_libraries="DoesNotExist"; +SET SESSION session_preload_libraries="DoesNotExist"; +begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback; -- clean up \c DROP SCHEMA test_roles_schema; diff --git a/src/test/modules/unsafe_tests/sql/rolenames.sql b/src/test/modules/unsafe_tests/sql/rolenames.sql index adac36536db..56fb52d4e08 100644 --- a/src/test/modules/unsafe_tests/sql/rolenames.sql +++ b/src/test/modules/unsafe_tests/sql/rolenames.sql @@ -494,6 +494,19 @@ RESET SESSION AUTHORIZATION; ROLLBACK; REVOKE pg_read_all_settings FROM regress_role_haspriv; +-- test some warnings +ALTER ROLE regress_testrol0 SET session_preload_libraries="DoesNotExist"; +ALTER ROLE regress_testrol0 SET local_preload_libraries="DoesNotExist"; +CREATE DATABASE regression_nosuch_db; +ALTER DATABASE regression_nosuch_db SET session_preload_libraries="DoesNotExist"; +ALTER DATABASE regression_nosuch_db SET local_preload_libraries="DoesNotExist"; +DROP DATABASE regression_nosuch_db; + +-- SET doesn't do anything, but should not warn, either +SET session_preload_libraries="DoesNotExist"; +SET SESSION session_preload_libraries="DoesNotExist"; +begin; SET LOCAL session_preload_libraries="DoesNotExist"; rollback; + -- clean up \c -- 2.42.0