Reject empty names and recursion in config-file include directives.

An empty file name or subdirectory name leads join_path_components() to
just produce the parent directory name, which leads to weird failures or
recursive inclusions.  Let's throw a specific error for that.  It takes
only slightly more code to detect all-blank names, so do so.

Also, detect direct recursion, ie a file calling itself.  As coded
this will also detect recursion via "include_dir '.'", which is
perhaps more likely than explicitly including the file itself.

Detecting indirect recursion would require API changes for guc-file.l
functions, which seems not worth it since extensions might call them.
The nesting depth limit will catch such cases eventually, just not
with such an on-point error message.

In passing, adjust the example usages in postgresql.conf.sample
to perhaps eliminate the problem at the source: there's no reason
for the examples to suggest that an empty value is valid.

Per a trouble report from Brent Bates.  Back-patch to 9.5; the
issue is old, but the code in 9.4 is enough different that the
patch doesn't apply easily, and it doesn't seem worth the trouble
to fix there.

Ian Barwick and Tom Lane

Discussion: https://postgr.es/m/8c8bcbca-3bd9-dc6e-8986-04a5abdef142@2ndquadrant.com

diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 1c8b5f7d8449607df26a72efbc18c231ad010c89..2aaab3d5f80a26359325a6ad275aa425b15ac806 100644 (file)
--- a/src/backend/utils/misc/guc-file.l
+++ b/src/backend/utils/misc/guc-file.l
@@ -567,6 +567,22 @@ ParseConfigFile(const char *config_file, bool strict,
        bool            OK = true;
        FILE       *fp;
 
+       /*
+        * Reject file name that is all-blank (including empty), as that leads to
+        * confusion --- we'd try to read the containing directory as a file.
+        */
+       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
@@ -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,28 @@ ParseConfigDirectory(const char *includedir,
        int                     size_filenames;
        bool            status;
 
+       /*
+        * Reject directory name that is all-blank (including empty), as that
+        * leads to confusion --- we'd read the containing directory, typically
+        * resulting in recursive inclusion of the same file(s).
+        */
+       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)

diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 93c745d6c908af72dc77619866f2fe26c54b9a5e..b61e66932ca544c82b524941f6d6fda9fec43bd7 100644 (file)
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -734,12 +734,13 @@
 #------------------------------------------------------------------------------
 
 # These options allow settings to be loaded from files other than the
-# default postgresql.conf.
+# default postgresql.conf.  Note that these are directives, not variable
+# assignments, so they can usefully be given more than once.
 
-#include_dir = ''                      # include files ending in '.conf' from
+#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
+#include_if_exists = '...'             # include file only if it exists
+#include = '...'                       # include file
 
 
 #------------------------------------------------------------------------------