[PATCH] Make configuration file "include" directive handling morerobust - Mailing list pgsql-hackers

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] https://www.postgresql.org/message-id/aed6cc9f-98f3-2693-ac81-52bb0052307e%402ndquadrant.com
    ("Stop ALTER SYSTEM from making bad assumptions")
[2]
https://www.postgresql.org/message-id/CY4PR1301MB22001D3FAAB3499C5D41DE23A9E50%40CY4PR1301MB2200.namprd13.prod.outlook.com
    ("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] 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


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.

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
previouslyparsed
 
     (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"
line8
 
    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