Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 6e42130

Browse files
committed
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
1 parent 9acda73 commit 6e42130

File tree

2 files changed

+63
-4
lines changed

2 files changed

+63
-4
lines changed

src/backend/utils/misc/guc-file.l

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,22 @@ ParseConfigFile(const char *config_file, bool strict,
567567
bool OK = true;
568568
FILE *fp;
569569

570+
/*
571+
* Reject file name that is all-blank (including empty), as that leads to
572+
* confusion --- we'd try to read the containing directory as a file.
573+
*/
574+
if (strspn(config_file, " \t\r\n") == strlen(config_file))
575+
{
576+
ereport(elevel,
577+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
578+
errmsg("empty configuration file name: \"%s\"",
579+
config_file)));
580+
record_config_file_error("empty configuration file name",
581+
calling_file, calling_lineno,
582+
head_p, tail_p);
583+
return false;
584+
}
585+
570586
/*
571587
* Reject too-deep include nesting depth. This is just a safety check to
572588
* avoid dumping core due to stack overflow if an include file loops back
@@ -585,6 +601,26 @@ ParseConfigFile(const char *config_file, bool strict,
585601
}
586602

587603
abs_path = AbsoluteConfigLocation(config_file, calling_file);
604+
605+
/*
606+
* Reject direct recursion. Indirect recursion is also possible, but it's
607+
* harder to detect and so doesn't seem worth the trouble. (We test at
608+
* this step because the canonicalization done by AbsoluteConfigLocation
609+
* makes it more likely that a simple strcmp comparison will match.)
610+
*/
611+
if (calling_file && strcmp(abs_path, calling_file) == 0)
612+
{
613+
ereport(elevel,
614+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
615+
errmsg("configuration file recursion in \"%s\"",
616+
calling_file)));
617+
record_config_file_error("configuration file recursion",
618+
calling_file, calling_lineno,
619+
head_p, tail_p);
620+
pfree(abs_path);
621+
return false;
622+
}
623+
588624
fp = AllocateFile(abs_path, "r");
589625
if (!fp)
590626
{
@@ -933,6 +969,28 @@ ParseConfigDirectory(const char *includedir,
933969
int size_filenames;
934970
bool status;
935971

972+
/*
973+
* Reject directory name that is all-blank (including empty), as that
974+
* leads to confusion --- we'd read the containing directory, typically
975+
* resulting in recursive inclusion of the same file(s).
976+
*/
977+
if (strspn(includedir, " \t\r\n") == strlen(includedir))
978+
{
979+
ereport(elevel,
980+
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
981+
errmsg("empty configuration directory name: \"%s\"",
982+
includedir)));
983+
record_config_file_error("empty configuration directory name",
984+
calling_file, calling_lineno,
985+
head_p, tail_p);
986+
return false;
987+
}
988+
989+
/*
990+
* We don't check for recursion or too-deep nesting depth here; the
991+
* subsequent calls to ParseConfigFile will take care of that.
992+
*/
993+
936994
directory = AbsoluteConfigLocation(includedir, calling_file);
937995
d = AllocateDir(directory);
938996
if (d == NULL)

src/backend/utils/misc/postgresql.conf.sample

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -734,12 +734,13 @@
734734
#------------------------------------------------------------------------------
735735

736736
# These options allow settings to be loaded from files other than the
737-
# default postgresql.conf.
737+
# default postgresql.conf. Note that these are directives, not variable
738+
# assignments, so they can usefully be given more than once.
738739

739-
#include_dir = '' # include files ending in '.conf' from
740+
#include_dir = '...' # include files ending in '.conf' from
740741
# a directory, e.g., 'conf.d'
741-
#include_if_exists = '' # include file only if it exists
742-
#include = '' # include file
742+
#include_if_exists = '...' # include file only if it exists
743+
#include = '...' # include file
743744

744745

745746
#------------------------------------------------------------------------------

0 commit comments

Comments
 (0)