From 47eb2ab393e050ac98a6197a8302c83c23ca0df3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
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 <unistd.h>
 
 /*
  * 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

