Ian Barwick <ian.barw...@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)
