Honor directive parameters after a quoted filename reference (#2457)

For example, the following ACL did not match `example.com`:

    acl foo dstdomain "file-of-domains.txt" example.com

strtokFile() implements the "read directive parameters from a quoted
file" syntax used when configuration_includes_quoted_values is off (the
documented default). Once the named file's lines were exhausted,
strtokFile() returned nullptr, signalling end-of-directive to callers
such as ACLDomainData::parse(). Any tokens following the quoted filename
on the same configuration line (e.g., "example.com" above) were silently
discarded. A parameter file reference and trailing literal parameters
are valid syntax individually; combining them on one line has always
been allowed by the grammar, so the omission was "invisible" to admins.

This bug dates back to 2008 commit f32cd13e ("Replace cnfig parser gotos
with do-while loop."), which replaced a goto that resumed parsing after
the file's last line with an unconditional "return nullptr". Every Squid
release starting with v3.1.0.1 is affected.

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index 23334bd09c38893ef2105b3e61bebecf192aa754..33d38274e48e3a7d5a1dc358280f86aa3ebef607 100644 (file)
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -367,6 +367,7 @@ Thank you!
     Michele Bergonzoni <bergonz@labs.it>
     Miguel A.L. Paraz <map@iphil.net>
     Mike Groeneweg <mikeg@scorpion.murdoch.edu.au>
+    Mike Lothian <mike@fireburn.co.uk>
     Mike Mitchell <mike.mitchell@sas.com>
     Mikio Kishi <mkishi@104.net>
     Milen Pankov <mail@milen.pankov.eu>

diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
index 6dfa92f3cfd9b899d2309b960e14c871f5c74fc3..90a5dc676c29b34415b1945e4605dcf72c7148d2 100644 (file)
--- a/src/ConfigParser.cc
+++ b/src/ConfigParser.cc
@@ -111,7 +111,9 @@ ConfigParser::strtokFile()
             fclose(wordFile);
             wordFile = nullptr;
             fromFile = 0;
-            return nullptr;
+            t = buf;
+            *t = '\0';
+            // and resume parsing post-"file" input, if any
         } else {
             char *t2, *t3;
             t = buf;

diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf
index 52f0a39c9255a8caf4cfe1e76647ec0a52973cc4..9807cf104b3f874de53d9c5183bb709197a12b22 100644 (file)
--- a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf
+++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf
@@ -17,3 +17,5 @@ acl test41 dstdomain .d.example.com .example.com
 acl test42 dstdomain .example.com .e.example.com
 
 acl test51 dstdomain example.com example.net .
+
+acl test61 dstdomain "bad-acl-dstdomain-dupe.domains" after-file.example.com

diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions
index ff18d9c66a7574f46fa07b662cf302fd37c3feab..e94a11534d20bbfb45bd625e550c64bdc56dd257 100644 (file)
--- a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions
+++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions
@@ -28,4 +28,8 @@ WARNING: Ignoring earlier .d.example.com because it is covered by .example.com
 WARNING: Ignoring .e.example.com because it is already covered by .example.com
     advice: Remove value .e.example.com from the ACL
     acl name: test42
+
+WARNING: Ignoring after-file.example.com because it is already covered by .example.com
+    advice: Remove value after-file.example.com from the ACL
+    acl name: test61
 END

diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.domains b/test-suite/squidconf/bad-acl-dstdomain-dupe.domains
new file mode 100644 (file)
index 0000000..c30ac31
--- /dev/null
+++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.domains
@@ -0,0 +1 @@
+.example.com