Re: [PATCH] Make configuration file "include" directive handling more robust - Mailing list pgsql-hackers

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

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)