[PATCH] Make configuration file "include" directive handling more robust

Started by Ian Barwickover 6 years ago5 messages

ian.barwick@2ndquadrant.com

over 6 years ago

7 attachment(s)

While poking about with [1]/messages/by-id/aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com ("Stop ALTER SYSTEM from making bad assumptions"), I noticed a few potential issues with the
inclusion handling for configuration files; another issue is demonstrated in [2]/messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com ("First Time Starting Up PostgreSQL and Having Problems").

[1]: /messages/by-id/aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com ("Stop ALTER SYSTEM from making bad assumptions")
("Stop ALTER SYSTEM from making bad assumptions")
[2]: /messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com ("First Time Starting Up PostgreSQL and Having Problems")
("First Time Starting Up PostgreSQL and Having Problems")

Specifically these are:

1) Provision of empty include directives
=========================================

The default postgresql.conf file includes these thusly:

#include_dir = '' # include files ending in '.conf' from
# a directory, e.g., 'conf.d'
#include_if_exists = '' # include file only if it exists
#include = '' # include file

Currently, uncommenting them but leaving the value empty (as happened in [2]/messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com ("First Time Starting Up PostgreSQL and Having Problems") above) can
result in unexpected behaviour.

For "include" and "include_if_exists", it's a not critical issue as non-existent
files are, well, non-existent, however this will leave behind the cryptic
message "input in flex scanner failed" in pg_file_settings's "error" column, e.g.:

However, an empty value for "include_dir" will result in the current configuration
file's directory being read, which can result in circular inclusion and triggering
of the nesting depth check.

Patch {1} makes provision of an empty value for any of these directives cause
configuration file processing to report an approprate error, e.g.:

2) Circular inclusion of configuration files
============================================

Currently there is a simple maximum nesting threshold (currently 10) which
will stop runaway circular inclusions. However, if triggered, it's not
always obvious what triggered it, and sometimes resource exhaustion
might kick in beforehand (as appeared to be the case in [2]/messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com ("First Time Starting Up PostgreSQL and Having Problems") above).

Patch {2} attempts to handle this situation by keeping track of which
files have already been included (based on their absolute, canonical
path) and reporting an error if they were encountered again.

On server startup:

2019-07-11 09:13:25.610 GMT [71140] LOG: configuration file "/var/lib/pgsql/data/postgresql.conf" was previously parsed
2019-07-11 09:13:25.610 GMT [71140] FATAL: configuration file "/var/lib/pgsql/data/postgresql.conf" contains errors

After sending SIGHUP:

postgres=# SELECT sourceline, seqno, name, setting, applied, error FROM pg_file_settings WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+--------------------------------------------------------------------------------
757 | 45 | | | f | configuration file "/var/lib/pgsql/data/postgresql.conf" was previously parsed
(1 row)

3) "include" directives in postgresql.auto.conf and extension control files
===========================================================================

Currently these are parsed and acted on, even though it makes no sense for further
config files to be included in either case.

With "postgresql.auto.conf", if a file is successfully included, its contents
will then be written to "postgresql.auto.conf" and the include directive will be
removed, which seems like a recipe for confusion.

These are admittedly unlikely corner cases, but it's easy enough to stop this
happening on the offchance someone tries to use this to solve some problem in
completely the wrong way.

Patch {3} implements this (note that this patch depends on patch {2}).

Extension example:

postgres=# CREATE EXTENSION repmgr;
ERROR: "include" not permitted in file "/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 8
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_dir" not permitted in file "/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 9
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_if_exists" not permitted in file "/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control" line 10

pg.auto.conf example:

postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
ERROR: could not parse contents of file "postgresql.auto.conf"
postgres=# SELECT regexp_replace(sourcefile, '^/.+/','') AS sourcefile,
seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourcefile | seqno | name | setting | applied | error
----------------------+-------+------+---------+---------+-------------------------
postgresql.auto.conf | 45 | | | f | "include" not permitted
(1 row)

The patch also has the side-effect that "include" directives are no longer
(silently) removed from "postgresql.auto.conf"; as the only way they can get
into the file in the first place is by manually editing it, I feel it's
reasonable for the user to be made aware that they're not valid and have to
manually remove them.

Patches
=======

Code:

{1} disallow-empty-include-directives.v1.patch
{2} track-included-files.v1.patch
{3} prevent-disallowed-includes.v1.patch

TAP tests:
{1} tap-test-configuration.v1.patch
{2} tap-test-disallow-empty-include-directives.v1.patch
{3} tap-test-track-included-files.v1.patch
{4} tap-test-prevent-disallowed-includes.v1.patch

Patches apply cleanly to REL_12_STABLE/HEAD, they could be modfied for
all supported versions if required. I can consolidate the patches
if preferred.

Regards

Ian Barwick

--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Attachments:

disallow-empty-include-directives.v1.patchtext/x-patch; name=disallow-empty-include-directives.v1.patchDownload

commit c46b50d2a1aa81e5457a61a5b3814e5a560e68d6
Author: Ian Barwick <barwick@gmail.com>
Date:   Tue Jul 16 00:01:20 2019 +0900

    Explicitly disallow empty "include" directives
    
    For "include" and "include_if_exists", provision of an empty value
    resulted in a rather cryptic "input in flex scanner failed" error being
    reported.
    
    For "include_dir", provision of an empty value would result in all
    configuration files in the same directory as the current file being
    parsed, probably resulting in circular inclusion.

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 1c8b5f7d84..807d950291 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -782,10 +782,17 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include_dir directive isn't a variable and should be
 			 * processed immediately.
 			 */
-			if (!ParseConfigDirectory(opt_value,
-									  config_file, ConfigFileLineno - 1,
-									  depth + 1, elevel,
-									  head_p, tail_p))
+			if (opt_value[0] == '\0')
+			{
+				record_config_file_error("\"include_dir\" must not be empty",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (!ParseConfigDirectory(opt_value,
+										   config_file, ConfigFileLineno - 1,
+										   depth + 1, elevel,
+										   head_p, tail_p))
 				OK = false;
 			yy_switch_to_buffer(lex_buffer);
 			pfree(opt_name);
@@ -797,7 +804,14 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include_if_exists directive isn't a variable and should be
 			 * processed immediately.
 			 */
-			if (!ParseConfigFile(opt_value, false,
+			if (opt_value[0] == '\0')
+			{
+				record_config_file_error("\"include_if_exists\" must not be empty",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (!ParseConfigFile(opt_value, false,
 								 config_file, ConfigFileLineno - 1,
 								 depth + 1, elevel,
 								 head_p, tail_p))
@@ -812,7 +826,14 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include directive isn't a variable and should be processed
 			 * immediately.
 			 */
-			if (!ParseConfigFile(opt_value, true,
+			if (opt_value[0] == '\0')
+			{
+				record_config_file_error("\"include\" must not be empty",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (!ParseConfigFile(opt_value, true,
 								 config_file, ConfigFileLineno - 1,
 								 depth + 1, elevel,
 								 head_p, tail_p))

track-included-files.v1.patchtext/x-patch; name=track-included-files.v1.patchDownload

commit fc89b8fff6e8c4f9c8247757440f5b9d78b77584
Author: Ian Barwick <barwick@gmail.com>
Date:   Mon Jul 15 15:19:41 2019 +0900

    Track which configuration files have already been included

diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index f7202cc9e7..7e8526617d 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -501,7 +501,7 @@ parse_extension_control_file(ExtensionControlFile *control,
 	 * Parse the file content, using GUC's file parsing code.  We need not
 	 * check the return value since any errors will be thrown at ERROR level.
 	 */
-	(void) ParseConfigFp(file, filename, 0, ERROR, &head, &tail);
+	(void) ParseConfigFp(file, filename, 0, ERROR, &head, &tail, NULL);
 
 	FreeFile(file);
 
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 807d950291..9ebdd62923 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -178,19 +178,25 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
 			   *tail;
 	int			i;
 
+	/* Ensure we don't parse the same file more than once */
+	List       *files_seen = NIL;
+
 	/* Parse the main config file into a list of option names and values */
 	ConfFileWithError = ConfigFileName;
 	head = tail = NULL;
 
 	if (!ParseConfigFile(ConfigFileName, true,
 						 NULL, 0, 0, elevel,
-						 &head, &tail))
+						 &head, &tail, &files_seen))
 	{
 		/* Syntax error(s) detected in the file, so bail out */
 		error = true;
 		goto bail_out;
 	}
 
+	if (files_seen)
+		list_free(files_seen);
+
 	/*
 	 * Parse the PG_AUTOCONF_FILENAME file, if present, after the main file to
 	 * replace any parameters set by ALTER SYSTEM command.  Because this file
@@ -201,7 +207,7 @@ ProcessConfigFileInternal(GucContext context, bool applySettings, int elevel)
 	{
 		if (!ParseConfigFile(PG_AUTOCONF_FILENAME, false,
 							 NULL, 0, 0, elevel,
-							 &head, &tail))
+							 &head, &tail, NULL))
 		{
 			/* Syntax error(s) detected in the file, so bail out */
 			error = true;
@@ -561,7 +567,8 @@ ParseConfigFile(const char *config_file, bool strict,
 				const char *calling_file, int calling_lineno,
 				int depth, int elevel,
 				ConfigVariable **head_p,
-				ConfigVariable **tail_p)
+				ConfigVariable **tail_p,
+				List **files_seen)
 {
 	char	   *abs_path;
 	bool		OK = true;
@@ -585,6 +592,35 @@ ParseConfigFile(const char *config_file, bool strict,
 	}
 
 	abs_path = AbsoluteConfigLocation(config_file, calling_file);
+
+	/*
+	 * If we've seen this file before, emit an error, otherwise
+	 * add it to the list of files seen.
+	 */
+	if (files_seen)
+	{
+		ListCell *lc;
+
+		foreach(lc, *files_seen)
+		{
+			char *file_path = (char *)lfirst(lc);
+			if (strncmp(file_path, abs_path, MAXPGPATH) == 0)
+			{
+				ereport(elevel,
+						(errcode(ERRCODE_CONFIG_FILE_ERROR),
+						 errmsg("configuration file \"%s\" was previously parsed",
+								abs_path)));
+				record_config_file_error(psprintf("configuration file \"%s\" was previously parsed",
+												  abs_path),
+										 calling_file, calling_lineno,
+										 head_p, tail_p);
+				return false;
+			}
+		}
+
+		*files_seen = lappend(*files_seen, pstrdup(abs_path));
+	}
+
 	fp = AllocateFile(abs_path, "r");
 	if (!fp)
 	{
@@ -609,7 +645,7 @@ ParseConfigFile(const char *config_file, bool strict,
 		goto cleanup;
 	}
 
-	OK = ParseConfigFp(fp, abs_path, depth, elevel, head_p, tail_p);
+	OK = ParseConfigFp(fp, abs_path, depth, elevel, head_p, tail_p, files_seen);
 
 cleanup:
 	if (fp)
@@ -696,7 +732,8 @@ GUC_flex_fatal(const char *msg)
  */
 bool
 ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
-			  ConfigVariable **head_p, ConfigVariable **tail_p)
+			  ConfigVariable **head_p, ConfigVariable **tail_p,
+			  List **files_seen)
 {
 	volatile bool OK = true;
 	unsigned int save_ConfigFileLineno = ConfigFileLineno;
@@ -792,7 +829,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			else if (!ParseConfigDirectory(opt_value,
 										   config_file, ConfigFileLineno - 1,
 										   depth + 1, elevel,
-										   head_p, tail_p))
+										   head_p, tail_p, files_seen))
 				OK = false;
 			yy_switch_to_buffer(lex_buffer);
 			pfree(opt_name);
@@ -814,7 +851,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			else if (!ParseConfigFile(opt_value, false,
 								 config_file, ConfigFileLineno - 1,
 								 depth + 1, elevel,
-								 head_p, tail_p))
+								 head_p, tail_p, files_seen))
 				OK = false;
 			yy_switch_to_buffer(lex_buffer);
 			pfree(opt_name);
@@ -836,7 +873,7 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			else if (!ParseConfigFile(opt_value, true,
 								 config_file, ConfigFileLineno - 1,
 								 depth + 1, elevel,
-								 head_p, tail_p))
+								 head_p, tail_p, files_seen))
 				OK = false;
 			yy_switch_to_buffer(lex_buffer);
 			pfree(opt_name);
@@ -944,7 +981,8 @@ ParseConfigDirectory(const char *includedir,
 					 const char *calling_file, int calling_lineno,
 					 int depth, int elevel,
 					 ConfigVariable **head_p,
-					 ConfigVariable **tail_p)
+					 ConfigVariable **tail_p,
+					 List **files_seen)
 {
 	char	   *directory;
 	DIR		   *d;
@@ -1042,7 +1080,7 @@ ParseConfigDirectory(const char *includedir,
 			if (!ParseConfigFile(filenames[i], true,
 								 calling_file, calling_lineno,
 								 depth, elevel,
-								 head_p, tail_p))
+								 head_p, tail_p, files_seen))
 			{
 				status = false;
 				goto cleanup;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc463601ff..aa8361710b 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -8060,7 +8060,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
 								AutoConfFileName)));
 
 			/* parse it */
-			if (!ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail))
+			if (!ParseConfigFp(infile, AutoConfFileName, 0, LOG, &head, &tail, NULL))
 				ereport(ERROR,
 						(errcode(ERRCODE_CONFIG_FILE_ERROR),
 						 errmsg("could not parse contents of file \"%s\"",
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index e709177c37..19c2d082d6 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -145,15 +145,18 @@ typedef struct ConfigVariable
 extern bool ParseConfigFile(const char *config_file, bool strict,
 							const char *calling_file, int calling_lineno,
 							int depth, int elevel,
-							ConfigVariable **head_p, ConfigVariable **tail_p);
+							ConfigVariable **head_p, ConfigVariable **tail_p,
+							List **files_seen);
 extern bool ParseConfigFp(FILE *fp, const char *config_file,
 						  int depth, int elevel,
-						  ConfigVariable **head_p, ConfigVariable **tail_p);
+						  ConfigVariable **head_p, ConfigVariable **tail_p,
+						  List **files_seen);
 extern bool ParseConfigDirectory(const char *includedir,
 								 const char *calling_file, int calling_lineno,
 								 int depth, int elevel,
 								 ConfigVariable **head_p,
-								 ConfigVariable **tail_p);
+								 ConfigVariable **tail_p,
+								 List **files_seen);
 extern void FreeConfigVariables(ConfigVariable *list);
 
 /*

prevent-disallowed-includes.v1.patchtext/x-patch; name=prevent-disallowed-includes.v1.patchDownload

commit 0aeb31f3e612202e75b88b858d01493139e2fef8
Author: Ian Barwick <barwick@gmail.com>
Date:   Mon Jul 15 23:25:55 2019 +0900

    Ensure "include" directives are only processed where appropriate
    
    This ensures any "include" directives present in postgresql.auto.conf
    or extension control files result in an error.

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 9ebdd62923..6a4382b787 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -550,7 +550,7 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)
 
 /*
  * Read and parse a single configuration file.  This function recurses
- * to handle "include" directives.
+ * to handle "include" directives, unless otherwise requested.
  *
  * If "strict" is true, treat failure to open the config file as an error,
  * otherwise just skip the file.
@@ -558,6 +558,10 @@ AbsoluteConfigLocation(const char *location, const char *calling_file)
  * calling_file/calling_lineno identify the source of the request.
  * Pass NULL/0 if not recursing from an inclusion request.
  *
+ * **files_seen is used to keep track of files which have already been parsed,
+ * to prevent circular inclusion. If set to NULL, it means the caller does
+ * not want include directives to be handled.
+ *
  * See ParseConfigFp for further details.  This one merely adds opening the
  * config file rather than working from a caller-supplied file descriptor,
  * and absolute-ifying the path name if necessary.
@@ -717,6 +721,10 @@ GUC_flex_fatal(const char *msg)
  * name-value pairs read from the input file(s) will be appended to the list.
  * Error reports will also be appended to the list, if elevel < ERROR.
  *
+ * **files_seen is used to keep track of files which have already been parsed,
+ * to prevent circular inclusion. If set to NULL, it means the caller does
+ * not want include directives to be handled.
+ *
  * Returns TRUE if successful, FALSE if an error occurred.  The error has
  * already been ereport'd, it is only necessary for the caller to clean up
  * its own state and release the ConfigVariable list.
@@ -819,7 +827,18 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include_dir directive isn't a variable and should be
 			 * processed immediately.
 			 */
-			if (opt_value[0] == '\0')
+			if (files_seen == NULL)
+			{
+				ereport(elevel,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("\"include_dir\" not permitted in file \"%s\" line %u",
+								config_file, ConfigFileLineno - 1)));
+				record_config_file_error("\"include_dir\" not permitted",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (opt_value[0] == '\0')
 			{
 				record_config_file_error("\"include_dir\" must not be empty",
 										 config_file, ConfigFileLineno - 1,
@@ -841,7 +860,18 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include_if_exists directive isn't a variable and should be
 			 * processed immediately.
 			 */
-			if (opt_value[0] == '\0')
+			if (files_seen == NULL)
+			{
+				ereport(elevel,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("\"include_if_exists\" not permitted in file \"%s\" line %u",
+								config_file, ConfigFileLineno - 1)));
+				record_config_file_error("\"include_if_exists\" not permitted",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (opt_value[0] == '\0')
 			{
 				record_config_file_error("\"include_if_exists\" must not be empty",
 										 config_file, ConfigFileLineno - 1,
@@ -863,7 +893,18 @@ ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
 			 * An include directive isn't a variable and should be processed
 			 * immediately.
 			 */
-			if (opt_value[0] == '\0')
+			if (files_seen == NULL)
+			{
+				ereport(elevel,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("\"include\" not permitted in file \"%s\" line %u",
+								config_file, ConfigFileLineno - 1)));
+				record_config_file_error("\"include\" not permitted",
+										 config_file, ConfigFileLineno - 1,
+										 head_p, tail_p);
+				OK = false;
+			}
+			else if (opt_value[0] == '\0')
 			{
 				record_config_file_error("\"include\" must not be empty",
 										 config_file, ConfigFileLineno - 1,

tap-test-configuration.v1.patchtext/x-patch; name=tap-test-configuration.v1.patchDownload

commit 4f8dacaa54f18bae608c660771fdd6dc41b73514
Author: Ian Barwick <ian@2ndquadrant.com>
Date:   Tue Jul 16 15:41:56 2019 +0900

    Add basic infrastructure for testing configuration files

diff --git a/src/test/Makefile b/src/test/Makefile
index efb206aa75..441d9541c4 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,7 +12,8 @@ subdir = src/test
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = perl regress isolation modules authentication recovery subscription
+SUBDIRS = perl regress isolation modules authentication recovery subscription \
+	configuration
 
 # Test suites that are not safe by default but can be run if selected
 # by the user via the whitespace-separated list in variable
diff --git a/src/test/configuration/.gitignore b/src/test/configuration/.gitignore
new file mode 100644
index 0000000000..871e943d50
--- /dev/null
+++ b/src/test/configuration/.gitignore
@@ -0,0 +1,2 @@
+# Generated by test suite
+/tmp_check/
diff --git a/src/test/configuration/Makefile b/src/test/configuration/Makefile
new file mode 100644
index 0000000000..69e96974af
--- /dev/null
+++ b/src/test/configuration/Makefile
@@ -0,0 +1,25 @@
+#-------------------------------------------------------------------------
+#
+# Makefile for src/test/configuration
+#
+# Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/configuration/Makefile
+#
+#-------------------------------------------------------------------------
+
+EXTRA_INSTALL=contrib/test_decoding
+
+subdir = src/test/recovery
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+check:
+	$(prove_check)
+
+installcheck:
+	$(prove_installcheck)
+
+clean distclean maintainer-clean:
+	rm -rf tmp_check
diff --git a/src/test/configuration/README b/src/test/configuration/README
new file mode 100644
index 0000000000..1796f5c5e1
--- /dev/null
+++ b/src/test/configuration/README
@@ -0,0 +1,25 @@
+src/test/configuration/README
+
+Regression tests for configuration files
+========================================
+
+This directory contains a test suite for configuration files.
+
+Running the tests
+=================
+
+NOTE: You must have given the --enable-tap-tests argument to configure.
+Also, to use "make installcheck", you must have built and installed
+contrib/test_decoding in addition to the core code.
+
+Run
+    make check
+or
+    make installcheck
+You can use "make installcheck" if you previously did "make install".
+In that case, the code in the installation tree is tested.  With
+"make check", a temporary installation tree is built from the current
+sources and then tested.
+
+Either way, this test initializes, starts, and stops several test Postgres
+clusters.
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 6019f37f91..bff3d507a9 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -529,6 +529,25 @@ sub append_conf
 
 =pod
 
+=item $node->read_conf(filename)
+
+A shortcut method to read the contents of a configuration file.
+
+Does no validation, parsing or sanity checking.
+
+=cut
+
+sub read_conf
+{
+	my ($self, $filename, $str) = @_;
+
+	my $conffile = $self->data_dir . '/' . $filename;
+
+	return TestLib::slurp_file($conffile);
+}
+
+=pod
+
 =item $node->backup(backup_name)
 
 Create a hot backup with B<pg_basebackup> in subdirectory B<backup_name> of

tap-test-disallow-empty-include-directives.v1.patchtext/x-patch; name=tap-test-disallow-empty-include-directives.v1.patchDownload

commit e93fcfd43fc18f74a2f03ae3eeace2ad064158bd
Author: Ian Barwick <ian@2ndquadrant.com>
Date:   Tue Jul 16 16:41:57 2019 +0900

    Add TAP test to disallow empty includes

diff --git a/src/test/configuration/t/001_disallow-empty-include-directives.pl b/src/test/configuration/t/001_disallow-empty-include-directives.pl
new file mode 100644
index 0000000000..3c21732c13
--- /dev/null
+++ b/src/test/configuration/t/001_disallow-empty-include-directives.pl
@@ -0,0 +1,72 @@
+# Test that empty include directives are disallowed
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+our $postgresql_conf = 'postgresql.conf';
+
+
+our ($expected, $result, $config_line);
+
+# Initialize a single node
+# ------------------------
+
+my $node = get_new_node('primary');
+$node->init();
+$node->start;
+
+# Get number of lines in current postgresql.conf
+my $conf = $node->read_conf($postgresql_conf);
+my @conf = split("\n", $conf);
+my $conf_lines = scalar @conf;
+
+my $include = <<'EO_CONF';
+include = ''
+include_if_exists = ''
+include_dir = ''
+EO_CONF
+
+$node->append_conf($postgresql_conf, $include);
+
+
+# 1. Check "include" directive
+# ----------------------------
+
+$config_line = $conf_lines + 1;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   '"include" must not be empty',
+   'Check "include" directive (filename only)');
+
+
+# 2. Check "include_if_exists" directive
+# ---------------------------------------
+
+$config_line = $conf_lines + 2;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   '"include_if_exists" must not be empty',
+   'Check "include" directive (filename only)');
+
+
+# 3. Check "include_dir" directive
+# --------------------------------
+
+$config_line = $conf_lines + 3;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   '"include_dir" must not be empty',
+   'Check "include" directive (filename only)');
+

tap-test-track-included-files.v1.patchtext/x-patch; name=tap-test-track-included-files.v1.patchDownload

commit 4ff17c7f2008f7e6716d10832571f3302b4e91e6
Author: Ian Barwick <ian@2ndquadrant.com>
Date:   Tue Jul 16 17:45:39 2019 +0900

    Add TAP test for configuration file inclusion tracking

diff --git a/src/test/configuration/t/002_track_included_files.pl b/src/test/configuration/t/002_track_included_files.pl
new file mode 100644
index 0000000000..3450c4c0ae
--- /dev/null
+++ b/src/test/configuration/t/002_track_included_files.pl
@@ -0,0 +1,113 @@
+# Test that a configuration file is not read more than once
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 5;
+
+our $postgresql_conf = 'postgresql.conf';
+our $include_first_conf = 'aaa.conf';
+
+our ($expected, $result, $config_line);
+
+# Initialize a single node
+# ------------------------
+
+my $node = get_new_node('primary');
+$node->init();
+$node->start;
+
+# Get number of lines in current postgresql.conf
+my $conf = $node->read_conf($postgresql_conf);
+my @conf = split("\n", $conf);
+my $conf_lines = scalar @conf;
+
+# Attempt to have the main postgresql.conf file include itself
+
+my $include = <<'EO_CONF';
+include = 'postgresql.conf'
+include = './postgresql.conf'
+include_if_exists = 'postgresql.conf'
+include_if_exists = './postgresql.conf'
+include_dir = '.'
+EO_CONF
+
+$node->append_conf($postgresql_conf, $include);
+
+$expected = sprintf(
+	'configuration file "%s/%s" was previously parsed',
+	$node->data_dir,
+	$postgresql_conf,
+);
+
+# 1. Check "include" directive (filename only)
+# --------------------------------------------
+
+$config_line = $conf_lines + 1;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   $expected,
+   'Check "include" directive (filename only)');
+
+# 2. Check "include" directive (filename with relative path)
+# ----------------------------------------------------------
+
+$config_line = $conf_lines + 2;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   $expected,
+   'Check "include" directive (filename with relative path)');
+
+
+# 3. Check "include_if_exists" directive (filename only)
+# ------------------------------------------------------
+
+$config_line = $conf_lines + 3;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   $expected,
+   'Check "include_if_exists" directive (filename only)');
+
+
+
+# 4. Check "include_if_exists" directive (filename with relative path)
+# --------------------------------------------------------------------
+
+$config_line = $conf_lines + 4;
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings WHERE sourceline = $config_line");
+
+is($result,
+   $expected,
+   'Check "include_if_exists" directive (filename with relative path)');
+
+
+# 5. Check "include_dir" directive
+# --------------------------------
+
+# Create a configuration file which will be read first
+# by "include_dir"
+$node->append_conf($include_first_conf, "include = 'postgresql.conf'\n");
+$node->append_conf($postgresql_conf, "include_dir = '.'\n");
+
+$result = $node->safe_psql('postgres',
+						   "SELECT error FROM pg_catalog.pg_file_settings ".
+						   " WHERE sourcefile LIKE '%$include_first_conf' ".
+						   "   AND sourceline = 1");
+is($result,
+   $expected,
+   'Check "include_dir" directive');
+
+
+

tap-test-prevent-disallowed-includes.v1.patchtext/x-patch; name=tap-test-prevent-disallowed-includes.v1.patchDownload

commit 96a47b09318b7a6b390ec1b93f8fed477e5c898e
Author: Ian Barwick <ian@2ndquadrant.com>
Date:   Wed Jul 17 10:57:06 2019 +0900

    Add TAP test for invalid "include" directives
    
    Check that "include" directives are rejected in:
    
     - postgresql.auto.conf
     - extension control files

diff --git a/src/test/configuration/t/003_forbid_include_directives.pl b/src/test/configuration/t/003_forbid_include_directives.pl
new file mode 100644
index 0000000000..1bd092eda6
--- /dev/null
+++ b/src/test/configuration/t/003_forbid_include_directives.pl
@@ -0,0 +1,131 @@
+# Test that include directives are rejected where not appropriate,
+# specifically in:
+#  - postgresql.auto.conf
+#  - extension control files
+
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 6;
+
+our $postgresql_auto_conf = 'postgresql.auto.conf';
+our $extension_name = 'dummy_extension';
+our ($result, $result_stdout, $result_stderr);
+our $fh;
+
+# Initialize a single node
+# ------------------------
+
+my $node = get_new_node('primary');
+$node->init();
+$node->start;
+
+our $file_settings_query = sprintf(
+    "SELECT error FROM pg_catalog.pg_file_settings WHERE sourcefile = '%s/%s' AND sourceline = 1",
+    $node->data_dir,
+    $postgresql_auto_conf,
+);
+
+our $extension_control_file = $node->safe_psql(
+    'postgres',
+    "SELECT setting || '/extension/${extension_name}.control' FROM pg_catalog.pg_config WHERE name = 'SHAREDIR'",
+);
+
+
+# 1. Check "include" is rejected in "postgresql.auto.conf"
+# --------------------------------------------------------
+
+$node->write_conf($postgresql_auto_conf,
+                  "include = 'postgresql.conf'\n");
+
+$result = $node->safe_psql('postgres', $file_settings_query);
+
+is($result,
+   '"include" not permitted',
+   'Check "include" directive is rejected');
+
+
+# 2. Check "include_if_exists" is rejected in "postgresql.auto.conf"
+# ------------------------------------------------------------------
+
+$node->write_conf($postgresql_auto_conf,
+                  "include_if_exists = 'postgresql.conf'\n");
+
+$result = $node->safe_psql('postgres', $file_settings_query);
+
+is($result,
+   '"include_if_exists" not permitted',
+   'Check "include_if_exists" directive is rejected');
+
+
+# 3. Check "include_dir" is rejected in "postgresql.auto.conf"
+# ------------------------------------------------------------
+
+$node->write_conf($postgresql_auto_conf,
+                  "include_dir = 'postgresql.conf'\n");
+
+$result = $node->safe_psql('postgres', $file_settings_query);
+
+is($result,
+   '"include_dir" not permitted',
+   'Check "include_dir" directive is rejected');
+
+
+# 4. Check "include" is rejected in an extension control file
+# ------------------------------------------------------------
+
+open $fh, ">", $extension_control_file,
+    or die "could not write \"$extension_control_file\": $!";
+
+print $fh "include = 'postgresql.conf'\n";
+close $fh;
+
+($result, $result_stdout, $result_stderr) = $node->psql(
+    'postgres',
+    "CREATE EXTENSION $extension_name",
+);
+
+like($result_stderr,
+   qr/"include" not permitted in file/,
+   'Check "include" directive is rejected');
+
+# 5. Check "include_if_exists" is rejected in an extension control file
+# ---------------------------------------------------------------------
+
+open $fh, ">", $extension_control_file,
+    or die "could not write \"$extension_control_file\": $!";
+
+print $fh "include_if_exists = 'postgresql.conf'\n";
+close $fh;
+
+($result, $result_stdout, $result_stderr) = $node->psql(
+    'postgres',
+    "CREATE EXTENSION $extension_name",
+);
+
+like($result_stderr,
+   qr/"include_if_exists" not permitted in file/,
+   'Check "include_if_exists" directive is rejected');
+
+
+# 6. Check "include_dir" is rejected in an extension control file
+# ---------------------------------------------------------------
+
+open $fh, ">", $extension_control_file,
+    or die "could not write \"$extension_control_file\": $!";
+
+print $fh "include_dir = 'postgresql.conf'\n";
+close $fh;
+
+($result, $result_stdout, $result_stderr) = $node->psql(
+    'postgres',
+    "CREATE EXTENSION $extension_name",
+);
+
+like($result_stderr,
+   qr/"include_dir" not permitted in file/,
+   'Check "include_dir" directive is rejected');
+
+# Remove the invalid extension control file
+unlink $extension_control_file;

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 6 years ago

In reply to: Ian Barwick (#1)

Re: [PATCH] Make configuration file "include" directive handling more robust

Hello.

At Wed, 17 Jul 2019 12:29:43 +0900, Ian Barwick <ian.barwick@2ndquadrant.com> wrote in <8c8bcbca-3bd9-dc6e-8986-04a5abdef142@2ndquadrant.com>

Hi

While poking about with [1], I noticed a few potential issues with the
inclusion handling for configuration files; another issue is
demonstrated in [2].

[1]
/messages/by-id/aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com
("Stop ALTER SYSTEM from making bad assumptions")
[2]
/messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com
("First Time Starting Up PostgreSQL and Having Problems")

Yeah! That's annoying..

Specifically these are:

1) Provision of empty include directives
=========================================

The default postgresql.conf file includes these thusly:

#include_dir = '' # include files ending in '.conf' from
# a directory, e.g., 'conf.d'
#include_if_exists = '' # include file only if it exists
#include = '' # include file

Currently, uncommenting them but leaving the value empty (as happened
in [2] above) can
result in unexpected behaviour.

For "include" and "include_if_exists", it's a not critical issue as
non-existent
files are, well, non-existent, however this will leave behind the
cryptic
message "input in flex scanner failed" in pg_file_settings's "error"
column, e.g.:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings
WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+------------------------------
1 | 45 | | | f | input in flex scanner failed
1 | 46 | | | f | input in flex scanner failed
(2 rows)

However, an empty value for "include_dir" will result in the current
configuration
file's directory being read, which can result in circular inclusion
and triggering
of the nesting depth check.

Patch {1} makes provision of an empty value for any of these
directives cause
configuration file processing to report an approprate error, e.g.:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings
WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+---------------------------------------
757 | 45 | | | f | "include" must not be empty
758 | 46 | | | f | "include_if_exists" must not be empty
759 | 47 | | | f | "include_dir" must not be empty

The patch 1 looks somewhat superficial. All the problems are
reduced to creating unexpected filepath for
inclusion. AbsoluteConfigLocation does the core work, and it can
issue generic error message covering all the cases like:

invalid parameter "<param>" at <calling_file>:<calling_lineno>

which seems sufficient. (The function needs some additional
parameters.)

2) Circular inclusion of configuration files
============================================

Currently there is a simple maximum nesting threshold (currently 10)
which
will stop runaway circular inclusions. However, if triggered, it's not
always obvious what triggered it, and sometimes resource exhaustion
might kick in beforehand (as appeared to be the case in [2] above).

Patch {2} attempts to handle this situation by keeping track of which
files have already been included (based on their absolute, canonical
path) and reporting an error if they were encountered again.

This seems to me to be overkill. The issue [2] is prevented by
the patch 1's amendment. (I don't think it's not worth donig to
add protection from explicit inclusion of pg_hba.conf from
postgresql.conf or itself or such like.)

On server startup:

2019-07-11 09:13:25.610 GMT [71140] LOG: configuration file
"/var/lib/pgsql/data/postgresql.conf" was previously parsed
2019-07-11 09:13:25.610 GMT [71140] FATAL: configuration file
"/var/lib/pgsql/data/postgresql.conf" contains errors

After sending SIGHUP:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+--------------------------------------------------------------------------------
757 | 45 | | | f | configuration file
"/var/lib/pgsql/data/postgresql.conf" was previously parsed
(1 row)

3) "include" directives in postgresql.auto.conf and extension control
files
===========================================================================

Currently these are parsed and acted on, even though it makes no sense
for further
config files to be included in either case.

Anyway manual edit is explicitly prohibited for auto.conf. And,
even if it is added, the 10-depth limitation would protect from
infinite loop.

With "postgresql.auto.conf", if a file is successfully included, its
contents
will then be written to "postgresql.auto.conf" and the include
directive will be
removed, which seems like a recipe for confusion.

These are admittedly unlikely corner cases, but it's easy enough to
stop this
happening on the offchance someone tries to use this to solve some
problem in
completely the wrong way.

Patch {3} implements this (note that this patch depends on patch {2}).

Extension example:

postgres=# CREATE EXTENSION repmgr;
ERROR: "include" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 8
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_dir" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 9
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_if_exists" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 10

pg.auto.conf example:

postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
ERROR: could not parse contents of file "postgresql.auto.conf"
postgres=# SELECT regexp_replace(sourcefile, '^/.+/','') AS
sourcefile,
seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourcefile | seqno | name | setting | applied | error
----------------------+-------+------+---------+---------+-------------------------
postgresql.auto.conf | 45 | | | f | "include" not permitted
(1 row)

The patch also has the side-effect that "include" directives are no
longer
(silently) removed from "postgresql.auto.conf"; as the only way they
can get
into the file in the first place is by manually editing it, I feel
it's
reasonable for the user to be made aware that they're not valid and
have to
manually remove them.

Patches
=======

Code:

{1} disallow-empty-include-directives.v1.patch
{2} track-included-files.v1.patch
{3} prevent-disallowed-includes.v1.patch

TAP tests:
{1} tap-test-configuration.v1.patch
{2} tap-test-disallow-empty-include-directives.v1.patch
{3} tap-test-track-included-files.v1.patch
{4} tap-test-prevent-disallowed-includes.v1.patch

Patches apply cleanly to REL_12_STABLE/HEAD, they could be modfied for
all supported versions if required. I can consolidate the patches
if preferred.

I don't think this is new to 12.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Ian Barwick

ian.barwick@2ndquadrant.com

over 6 years ago

In reply to: Kyotaro Horiguchi (#2)

Re: [PATCH] Make configuration file "include" directive handling more robust

On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello.

At Wed, 17 Jul 2019 12:29:43 +0900, Ian Barwick <ian.barwick@2ndquadrant.com> wrote in <8c8bcbca-3bd9-dc6e-8986-04a5abdef142@2ndquadrant.com>

Hi

While poking about with [1], I noticed a few potential issues with the
inclusion handling for configuration files; another issue is
demonstrated in [2].

[1]
/messages/by-id/aed6cc9f-98f3-2693-ac81-52bb0052307e@2ndquadrant.com
("Stop ALTER SYSTEM from making bad assumptions")
[2]
/messages/by-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50@CY4PR1301MB2200.namprd13.prod.outlook.com
("First Time Starting Up PostgreSQL and Having Problems")

Yeah! That's annoying..

Specifically these are:

1) Provision of empty include directives
=========================================

The default postgresql.conf file includes these thusly:

#include_dir = '' # include files ending in '.conf' from
# a directory, e.g., 'conf.d'
#include_if_exists = '' # include file only if it exists
#include = '' # include file

Currently, uncommenting them but leaving the value empty (as happened
in [2] above) can
result in unexpected behaviour.

For "include" and "include_if_exists", it's a not critical issue as
non-existent
files are, well, non-existent, however this will leave behind the
cryptic
message "input in flex scanner failed" in pg_file_settings's "error"
column, e.g.:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings
WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+------------------------------
1 | 45 | | | f | input in flex scanner failed
1 | 46 | | | f | input in flex scanner failed
(2 rows)

However, an empty value for "include_dir" will result in the current
configuration
file's directory being read, which can result in circular inclusion
and triggering
of the nesting depth check.

Patch {1} makes provision of an empty value for any of these
directives cause
configuration file processing to report an approprate error, e.g.:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings
WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+---------------------------------------
757 | 45 | | | f | "include" must not be empty
758 | 46 | | | f | "include_if_exists" must not be empty
759 | 47 | | | f | "include_dir" must not be empty

The patch 1 looks somewhat superficial. All the problems are
reduced to creating unexpected filepath for
inclusion. AbsoluteConfigLocation does the core work, and it can
issue generic error message covering all the cases like:

invalid parameter "<param>" at <calling_file>:<calling_lineno>

which seems sufficient. (The function needs some additional
parameters.)

That seems unnecessarily complex to me, as we'd be overloading a
function with a single purpose (to manipulate a path) with some
of the parsing logic/control.

2) Circular inclusion of configuration files
============================================

Currently there is a simple maximum nesting threshold (currently 10)
which
will stop runaway circular inclusions. However, if triggered, it's not
always obvious what triggered it, and sometimes resource exhaustion
might kick in beforehand (as appeared to be the case in [2] above).

Patch {2} attempts to handle this situation by keeping track of which
files have already been included (based on their absolute, canonical
path) and reporting an error if they were encountered again.

This seems to me to be overkill. The issue [2] is prevented by
the patch 1's amendment.

Yes, that particular issue is prevented, but this patch is intended to
provide better protection against explicit circular inclusions, e.g.
if someone adds "include 'postgresql.conf'" at the end of "postgresql.conf"
(or more realistically has a complex setup with multiple included
configuration files where something gets mixed up).

Currently the nesting threshold stops it becoming a runaway
problem, but is not very user-friendly. E.g. with "include 'postgresql.conf'"
added to the end of "postgresql.conf", without patch on startup you get:

LOG: could not open configuration file "postgresql.conf": maximum nesting depth exceeded
FATAL: configuration file "/path/to/postgresql.conf" contains errors

(cue panicking user with production server down: "OMG the file can't be opened,
is my filesystem corrupted?" etc.)

With the patch:

LOG: configuration file "/path/to/postgresql.conf" was previously parsed
FATAL: configuration file "/path/to/postgresql.conf" contains errors

(actually maybe we could add a bit more detail such as line number there).

(I don't think it's not worth donig to
add protection from explicit inclusion of pg_hba.conf from
postgresql.conf or itself or such like.)

I thought about that, but came to the same conclusion.

On server startup:

2019-07-11 09:13:25.610 GMT [71140] LOG: configuration file
"/var/lib/pgsql/data/postgresql.conf" was previously parsed
2019-07-11 09:13:25.610 GMT [71140] FATAL: configuration file
"/var/lib/pgsql/data/postgresql.conf" contains errors

After sending SIGHUP:

postgres=# SELECT sourceline, seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourceline | seqno | name | setting | applied | error
------------+-------+------+---------+---------+--------------------------------------------------------------------------------
757 | 45 | | | f | configuration file
"/var/lib/pgsql/data/postgresql.conf" was previously parsed
(1 row)

3) "include" directives in postgresql.auto.conf and extension control
files
===========================================================================

Currently these are parsed and acted on, even though it makes no sense
for further
config files to be included in either case.

Anyway manual edit is explicitly prohibited for auto.conf.

Indeed, but there are many things we tell people not to do, such as
removing tablespace directories, but they still do them...

And even if it is added, the 10-depth limitation would protect from
infinite loop.

It's not just about protecting againt infinite loops - if you do something
like "include 'postgresql.conf'", as-is the code will happily slurp in
all the items from "postgresql.conf" into "postgresql.auto.conf", which
is going to cause some headscratching if it ever happens.

Like I said in the original mail these are extremely unlikely corner cases;
but if patch {2} is in place, it's trivial to prevent them ever becoming a
problem.

With "postgresql.auto.conf", if a file is successfully included, its
contents
will then be written to "postgresql.auto.conf" and the include
directive will be
removed, which seems like a recipe for confusion.

These are admittedly unlikely corner cases, but it's easy enough to
stop this
happening on the offchance someone tries to use this to solve some
problem in
completely the wrong way.

Patch {3} implements this (note that this patch depends on patch {2}).

Extension example:

postgres=# CREATE EXTENSION repmgr;
ERROR: "include" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 8
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_dir" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 9
postgres=# CREATE EXTENSION repmgr;
ERROR: "include_if_exists" not permitted in file
"/home/barwick/devel/postgres/builds/HEAD/share/extension/repmgr.control"
line 10

pg.auto.conf example:

postgres=# ALTER SYSTEM SET default_tablespace ='pg_default';
ERROR: could not parse contents of file "postgresql.auto.conf"
postgres=# SELECT regexp_replace(sourcefile, '^/.+/','') AS
sourcefile,
seqno, name, setting, applied, error
FROM pg_file_settings WHERE error IS NOT NULL;
sourcefile | seqno | name | setting | applied | error
----------------------+-------+------+---------+---------+-------------------------
postgresql.auto.conf | 45 | | | f | "include" not permitted
(1 row)

The patch also has the side-effect that "include" directives are no
longer
(silently) removed from "postgresql.auto.conf"; as the only way they
can get
into the file in the first place is by manually editing it, I feel
it's
reasonable for the user to be made aware that they're not valid and
have to
manually remove them.

Patches
=======

Code:

{1} disallow-empty-include-directives.v1.patch
{2} track-included-files.v1.patch
{3} prevent-disallowed-includes.v1.patch

TAP tests:
{1} tap-test-configuration.v1.patch
{2} tap-test-disallow-empty-include-directives.v1.patch
{3} tap-test-track-included-files.v1.patch
{4} tap-test-prevent-disallowed-includes.v1.patch

Patches apply cleanly to REL_12_STABLE/HEAD, they could be modfied for
all supported versions if required. I can consolidate the patches
if preferred.

I don't think this is new to 12.

No, though I'm not sure how much this would be seen as a bugfix
and how far back it would be sensible to patch.

Regards

Ian Barwick

--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Tom Lane

tgl@sss.pgh.pa.us

over 6 years ago

In reply to: Ian Barwick (#3)

1 attachment(s)

Re: [PATCH] Make configuration file "include" directive handling more robust

Ian Barwick <ian.barwick@2ndquadrant.com> writes:

On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello.

I don't think this is new to 12.

No, though I'm not sure how much this would be seen as a bugfix
and how far back it would be sensible to patch.

I think this is worth considering as a bugfix; although I'm afraid
we can't change the signature of ParseConfigFile/ParseConfigFp in
released branches, since extensions could possibly be using those.
That limits what we can do --- but it's still possible to detect
direct recursion, which seems like enough to produce a nice error
message in typical cases.

I concur with Kyotaro-san that disallow-empty-include-directives.v1.patch
seems a bit brute-force, but where I would put the checks is in
ParseConfigFile and ParseConfigDirectory.

Also, I don't agree with the goals of prevent-disallowed-includes.patch.
I'm utterly not on board with breaking use of "include" in extension
files, for instance; while that may not be documented, it works fine,
and maybe somebody out there is relying on it. Likewise, while "include"
in pg.auto.conf is not really considered supported, I don't see the
point of going out of our way to break the historical behavior.

That leads me to propose the attached simplified patch. While I haven't
actually tried, I'm pretty sure this should back-patch without trouble.

regards, tom lane

Attachments:

tighten-up-config-file-inclusions-1.patchtext/x-diff; charset=us-ascii; name=tighten-up-config-file-inclusions-1.patchDownload

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 1c8b5f7..125b898 100644
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -568,6 +568,22 @@ ParseConfigFile(const char *config_file, bool strict,
 	FILE	   *fp;
 
 	/*
+	 * Reject file name that is all-blank (including empty), as that leads to
+	 * confusion, or even recursive inclusion in some cases.
+	 */
+	if (strspn(config_file, " \t\r\n") == strlen(config_file))
+	{
+		ereport(elevel,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("empty configuration file name: \"%s\"",
+						config_file)));
+		record_config_file_error("empty configuration file name",
+								 calling_file, calling_lineno,
+								 head_p, tail_p);
+		return false;
+	}
+
+	/*
 	 * Reject too-deep include nesting depth.  This is just a safety check to
 	 * avoid dumping core due to stack overflow if an include file loops back
 	 * to itself.  The maximum nesting depth is pretty arbitrary.
@@ -585,6 +601,26 @@ ParseConfigFile(const char *config_file, bool strict,
 	}
 
 	abs_path = AbsoluteConfigLocation(config_file, calling_file);
+
+	/*
+	 * Reject direct recursion.  Indirect recursion is also possible, but it's
+	 * harder to detect and so doesn't seem worth the trouble.  (We test at
+	 * this step because the canonicalization done by AbsoluteConfigLocation
+	 * makes it more likely that a simple strcmp comparison will match.)
+	 */
+	if (calling_file && strcmp(abs_path, calling_file) == 0)
+	{
+		ereport(elevel,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("configuration file recursion in \"%s\"",
+						calling_file)));
+		record_config_file_error("configuration file recursion",
+								 calling_file, calling_lineno,
+								 head_p, tail_p);
+		pfree(abs_path);
+		return false;
+	}
+
 	fp = AllocateFile(abs_path, "r");
 	if (!fp)
 	{
@@ -933,6 +969,27 @@ ParseConfigDirectory(const char *includedir,
 	int			size_filenames;
 	bool		status;
 
+	/*
+	 * Reject directory name that is all-blank (including empty), as that
+	 * leads to confusion, or even recursive inclusion in some cases.
+	 */
+	if (strspn(includedir, " \t\r\n") == strlen(includedir))
+	{
+		ereport(elevel,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("empty configuration directory name: \"%s\"",
+						includedir)));
+		record_config_file_error("empty configuration directory name",
+								 calling_file, calling_lineno,
+								 head_p, tail_p);
+		return false;
+	}
+
+	/*
+	 * We don't check for recursion or too-deep nesting depth here; the
+	 * subsequent calls to ParseConfigFile will take care of that.
+	 */
+
 	directory = AbsoluteConfigLocation(includedir, calling_file);
 	d = AllocateDir(directory);
 	if (d == NULL)

Ian Barwick

ian.barwick@2ndquadrant.com

over 6 years ago

In reply to: Tom Lane (#4)

Re: [PATCH] Make configuration file "include" directive handling more robust

On 8/25/19 4:39 AM, Tom Lane wrote:

Ian Barwick <ian.barwick@2ndquadrant.com> writes:

On 7/17/19 5:34 PM, Kyotaro Horiguchi wrote:> Hello.

I don't think this is new to 12.

No, though I'm not sure how much this would be seen as a bugfix
and how far back it would be sensible to patch.

I think this is worth considering as a bugfix; although I'm afraid
we can't change the signature of ParseConfigFile/ParseConfigFp in
released branches, since extensions could possibly be using those.
That limits what we can do --- but it's still possible to detect
direct recursion, which seems like enough to produce a nice error
message in typical cases.

Makes sense.

I concur with Kyotaro-san that disallow-empty-include-directives.v1.patch
seems a bit brute-force, but where I would put the checks is in
ParseConfigFile and ParseConfigDirectory.

Also, I don't agree with the goals of prevent-disallowed-includes.patch.
I'm utterly not on board with breaking use of "include" in extension
files, for instance; while that may not be documented, it works fine,
and maybe somebody out there is relying on it.

I couldn't for the life of me think of any reason for using it.
But if there's undocumented functionality we think someone might
be using, shouldn't that be documented somewhere, if only as a note
in the code to prevent its accidental removal at a later date?

Likewise, while "include"
in pg.auto.conf is not really considered supported, I don't see the
point of going out of our way to break the historical behavior.

The amusing thing about that of course is that the include directive
will disappear the next time ALTER SYSTEM is run and the values from
the included file will appear in pg.auto.conf, which may cause some
headscratching. But I guess hasn't been an actual real-world
issue so far.

That leads me to propose the attached simplified patch. While I haven't
actually tried, I'm pretty sure this should back-patch without trouble.

Ah, I see it's been applied already, thanks!

Regards

Ian Barwick

--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services