Remove AclMatchedName from ACL::ParseAclLine()

src/acl/Acl.cc

@@ -102,6 +102,18 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey)
                         Here());
 }
 
+ScopedId
+Acl::ParsingContext::codeContextGist() const {
+    return ScopedId("acl");
+}
+
+std::ostream &
+Acl::ParsingContext::detailCodeContext(std::ostream &os) const
+{
+    return os << Debug::Extra << "acl name: " << name_ <<
+        Debug::Extra << "configuration context: " << ConfigParser::CurrentLocation();
+}
+
 void *
 ACL::operator new (size_t)
 {
@@ -193,9 +205,7 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
 {
     /* we're already using strtok() to grok the line */
     char *t = nullptr;
-    ACL *A = nullptr;
     LOCAL_ARRAY(char, aclname, ACL_NAME_SZ);
-    int new_acl = 0;
 
     /* snarf the ACL name */
 
@@ -213,6 +223,15 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
     }
 
     xstrncpy(aclname, t, ACL_NAME_SZ);
+
+    CallContextParser(Acl::ParsingContext::Pointer::Make(aclname), [&] {
+        ParseNamed(parser, head, aclname);
+    });
+}
+
+void
+ACL::ParseNamed(ConfigParser &parser, ACL ** head, const char *aclname)
+{
     /* snarf the ACL type */
     const char *theType;
 
@@ -253,6 +272,8 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
         theType = "client_connection_mark";
     }
 
+    ACL *A = nullptr;
+    int new_acl = 0;
     if ((A = FindByName(aclname)) == nullptr) {
         debugs(28, 3, "aclParseAclLine: Creating ACL '" << aclname << "'");
         A = Acl::Make(theType);
@@ -269,22 +290,11 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
         new_acl = 0;
     }
 
-    /*
-     * Here we set AclMatchedName in case we need to use it in a
-     * warning message in aclDomainCompare().
-     */
-    AclMatchedName = A->name;   /* ugly */
-
     A->parseFlags();
 
     /*split the function here */
     A->parse();
 
-    /*
-     * Clear AclMatchedName from our temporary hack
-     */
-    AclMatchedName = nullptr;  /* ugly */
-
     if (!new_acl)
         return;
 

src/acl/Acl.h

@@ -11,6 +11,7 @@
 
 #include "acl/forward.h"
 #include "acl/Options.h"
+#include "base/CodeContext.h"
 #include "cbdata.h"
 #include "defines.h"
 #include "dlink.h"
@@ -36,6 +37,22 @@ void RegisterMaker(TypeName typeName, Maker maker);
 /// Key comparison is case-insensitive.
 void SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey);
 
+/// CodeContext of the ACL being parsed
+class ParsingContext : public CodeContext
+{
+public:
+    using Pointer = RefCount<ParsingContext>;
+
+    ParsingContext(const char *name) : name_(name) {}
+
+    /* CodeContext API */
+    ScopedId codeContextGist() const override;
+    std::ostream &detailCodeContext(std::ostream &os) const override;
+
+private:
+    const char *name_; ///< the parsed ACL name
+};
+
 } // namespace Acl
 
 /// A configurable condition. A node in the ACL expression tree.
@@ -111,6 +128,9 @@ class ACL
     /// \returns (linked) "line" Options supported by this ACL
     /// \see ACL::options()
     virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); }
+
+    /// parses the rest of ACL line following ACL name
+    static void ParseNamed(ConfigParser &, ACL ** head, const char *name);
 };
 
 /// \ingroup ACLAPI

src/acl/DomainData.cc

@@ -12,9 +12,11 @@
 #include "acl/Checklist.h"
 #include "acl/DomainData.h"
 #include "anyp/Uri.h"
+#include "base/TextException.h"
 #include "cache_cf.h"
 #include "ConfigParser.h"
 #include "debug/Stream.h"
+#include "sbuf/Stream.h"
 #include "util.h"
 
 template<class T>
@@ -78,24 +80,27 @@ aclDomainCompare(T const &a, T const &b)
             // When a.example.com comes after .example.com in an ACL
             // sub-domain is ignored. That is okay. Just important
             bool d3big = (strlen(d3) > strlen(d4)); // Always suggest removing the longer one.
-            debugs(28, DBG_IMPORTANT, "WARNING: '" << (d3big?d3:d4) << "' is a subdomain of '" << (d3big?d4:d3) << "'");
-            debugs(28, DBG_IMPORTANT, "WARNING: You should remove '" << (d3big?d3:d4) << "' from the ACL named '" << AclMatchedName << "'");
+            const auto longer = d3big ? d3 : d4;
+            const auto shorter = d3big ? d4 : d3;
+            debugs(28, DBG_IMPORTANT, "WARNING: '" << longer << "' is a subdomain of '" << shorter << "'" <<
+                    Debug::Extra << "You should remove '" << longer << "' from the ACL");
             debugs(28, 2, "Ignore '" << d3 << "' to keep splay tree searching predictable");
         }
     } else if (ret == 0) {
         // It may be an exact duplicate. No problem. Just drop.
         if (strcmp(d1,d2)==0) {
-            debugs(28, 2, "WARNING: '" << d2 << "' is duplicated in the list.");
-            debugs(28, 2, "WARNING: You should remove one '" << d2 << "' from the ACL named '" << AclMatchedName << "'");
+            debugs(28, DBG_PARSE_NOTE(2), "WARNING: '" << d2 << "' is duplicated in the list." <<
+                    Debug::Extra << "You should remove one '" << d2 << "' from the ACL");
             return ret;
         }
         // When a.example.com comes before .example.com in an ACL
         // discarding the wildcard is critically bad.
         // or Maybe even both are wildcards. Things are very weird in those cases.
         bool d1big = (strlen(d1) > strlen(d2)); // Always suggest removing the longer one.
-        debugs(28, DBG_CRITICAL, "ERROR: '" << (d1big?d1:d2) << "' is a subdomain of '" << (d1big?d2:d1) << "'");
-        debugs(28, DBG_CRITICAL, "ERROR: You need to remove '" << (d1big?d1:d2) << "' from the ACL named '" << AclMatchedName << "'");
-        self_destruct();
+        const auto longer = d1big ? d1 : d2;
+        const auto shorter = d1big ? d2 : d1;
+        throw TextException(ToSBuf("'", longer, "' is a subdomain of '", shorter, "'",
+                    Debug::Extra, "You need to remove '", longer, "' from the ACL"), Here());
     }
 
     return ret;

src/acl/HttpStatus.cc

@@ -48,9 +48,9 @@ int acl_httpstatus_data::compare(acl_httpstatus_data* const& a, acl_httpstatus_d
     if (ret == 0) {
         const SBuf sa = a->toStr();
         const SBuf sb = b->toStr();
-        debugs(28, DBG_CRITICAL, "WARNING: '" << sa << "' is a subrange of '" << sb << "'");
-        debugs(28, DBG_CRITICAL, "WARNING: because of this '" << sa << "' is ignored to keep splay tree searching predictable");
-        debugs(28, DBG_CRITICAL, "WARNING: You should probably remove '" << sb << "' from the ACL named '" << AclMatchedName << "'");
+        debugs(28, DBG_CRITICAL, "WARNING: '" << sa << "' is a subrange of '" << sb << "'" <<
+                Debug::Extra << "Because of this '" << sa << "' is ignored to keep splay tree searching predictable" <<
+                Debug::Extra << "You should probably remove '" << sb << "' from the ACL");
     }
 
     return ret;

src/acl/Ip.cc

@@ -139,9 +139,9 @@ acl_ip_data::NetworkCompare(acl_ip_data * const & a, acl_ip_data * const &b)
             a->toStr(buf_n1, 3*(MAX_IPSTRLEN+1));
             b->toStr(buf_n2, 3*(MAX_IPSTRLEN+1));
         }
-        debugs(28, DBG_CRITICAL, "WARNING: (" << (bina?'B':'A') << ") '" << buf_n1 << "' is a subnetwork of (" << (bina?'A':'B') << ") '" << buf_n2 << "'");
-        debugs(28, DBG_CRITICAL, "WARNING: because of this '" << (bina?buf_n2:buf_n1) << "' is ignored to keep splay tree searching predictable");
-        debugs(28, DBG_CRITICAL, "WARNING: You should probably remove '" << buf_n1 << "' from the ACL named '" << AclMatchedName << "'");
+        debugs(28, DBG_CRITICAL, "WARNING: (" << (bina?'B':'A') << ") '" << buf_n1 << "' is a subnetwork of (" << (bina?'A':'B') << ") '" << buf_n2 << "'" <<
+                Debug::Extra << "Because of this '" << (bina?buf_n2:buf_n1) << "' is ignored to keep splay tree searching predictable" <<
+                Debug::Extra << "You should probably remove '" << buf_n1 << "' from the ACL");
     }
 
     return ret;

src/acl/ProtocolData.cc

@@ -57,7 +57,7 @@ ACLProtocolData::parse()
             }
         }
         if (p == AnyP::PROTO_UNKNOWN) {
-            debugs(28, DBG_IMPORTANT, "WARNING: Ignoring unknown protocol '" << t << "' in the ACL named '" << AclMatchedName << "'");
+            debugs(28, DBG_IMPORTANT, "WARNING: Ignoring unknown protocol '" << t << "' in the ACL");
             // XXX: store the text pattern of this protocol name for live comparisons
         }
     }

src/base/CodeContext.h

@@ -106,20 +106,38 @@ class CodeContextGuard
     CodeContext::Pointer savedCodeContext;
 };
 
-/// Executes service `callback` in `callbackContext`. If an exception occurs,
-/// the callback context is preserved, so that the exception is associated with
-/// the callback that triggered them (rather than with the service).
+/// Executes 'fun' in `funContext`. If an exception occurs,
+/// the funContext is preserved, so that the exception is associated with
+/// the call that triggered it.
+template <typename Fun>
+inline void
+CallAndRestore_(const CodeContext::Pointer &funContext, Fun &&fun)
+{
+    const auto savedCodeContext(CodeContext::Current());
+    CodeContext::Reset(funContext);
+    fun();
+    CodeContext::Reset(savedCodeContext);
+}
+
+/// \copydoc CallAndRestore_(const CodeContext::Pointer &funContext, Fun &&fun)
 ///
 /// Service code running in its own service context should use this function.
 template <typename Fun>
 inline void
 CallBack(const CodeContext::Pointer &callbackContext, Fun &&callback)
 {
     // TODO: Consider catching exceptions and letting CodeContext handle them.
-    const auto savedCodeContext(CodeContext::Current());
-    CodeContext::Reset(callbackContext);
-    callback();
-    CodeContext::Reset(savedCodeContext);
+    CallAndRestore_(callbackContext, callback);
+}
+
+/// \copydoc CallAndRestore_(const CodeContext::Pointer &funContext, Fun &&fun)
+///
+/// Specific parser code initiated by parse_line() should use this function.
+template <typename Fun>
+inline void
+CallContextParser(const CodeContext::Pointer &parserContext, Fun &&parse)
+{
+    CallAndRestore_(parserContext, parse);
 }
 
 /// Executes `service` in `serviceContext` but due to automatic caller context

src/external_acl.cc

@@ -528,7 +528,7 @@ ACLExternal::parse()
 
     // def->name is the name of the external_acl_type.
     // this is the name of the 'acl' directive being tested
-    data->name = xstrdup(AclMatchedName);
+    data->name = xstrdup(name);
 
     while ((token = ConfigParser::strtokFile())) {
         wordlistAdd(&data->arguments, token);

test-suite/squidconf/acl-src-dstdomain-http-status.conf

@@ -0,0 +1,20 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo_dstdomain dstdomain example.com one.example.org
+acl foo_dstdomain dstdomain .example.net two.example.org
+acl foo_dstdomain dstdomain .three.example.org
+
+acl foo_src_4 src 192.168.2.0/24 192.168.1.0/24
+acl foo_src_4 src 192.168.3.0-192.168.3.255
+
+# TODO: make configurable depending on USE_IPV6
+# acl foo_src_6 src fed0::2-fee0:: fee0::1
+# acl foo_src_6 src fec0::-fed0:: fed0::1
+
+acl foo http_status 404 200 199
+acl foo http_status 500 206

test-suite/squidconf/bad-acl-dstdomain-dup.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain .example.com .example.com

test-suite/squidconf/bad-acl-dstdomain-dup.conf.instructions

@@ -0,0 +1 @@
+expect-message WARNING:.*example.com..is.duplicated.in.the.list

test-suite/squidconf/bad-acl-dstdomain-subdomain-after.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain .example.com a.example.com

test-suite/squidconf/bad-acl-dstdomain-subdomain-after.conf.instructions

@@ -0,0 +1 @@
+expect-message WARNING:..a.example.com..is.a.subdomain.of.*example.com

test-suite/squidconf/bad-acl-dstdomain-subdomain-before.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo dstdomain a.example.com .example.com

test-suite/squidconf/bad-acl-dstdomain-subdomain-before.conf.instructions

@@ -0,0 +1 @@
+expect-failure ERROR:.configuration.failure:..a.example.com..is.a.subdomain.of.*example.com

test-suite/squidconf/bad-acl-http-status-dup.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo http_status 200 200

test-suite/squidconf/bad-acl-http-status-dup.conf.instructions

@@ -0,0 +1 @@
+expect-message WARNING:..200..is.a.subrange.of..200

test-suite/squidconf/bad-acl-src-subnetwork-range-v6.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo src fec0::-fed0:: fec0::/10

test-suite/squidconf/bad-acl-src-subnetwork-range-v6.conf.instructions

@@ -0,0 +1,2 @@
+skip-unless-autoconf-defines USE_IPV6 1
+expect-message WARNING:.*fec0::-fed0::..is.a.subnetwork.of.*fec0::/10

test-suite/squidconf/bad-acl-src-subnetwork-range.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo src 192.168.0.0/16 192.168.1.0-192.168.1.255

test-suite/squidconf/bad-acl-src-subnetwork-range.conf.instructions

@@ -0,0 +1 @@
+expect-message WARNING:.*192.168.1.0-192.168.1.255..is.a.subnetwork.of.*192.168.0.0/16

test-suite/squidconf/bad-acl-src-subnetwork-v6.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo src fec0::/10 fee0::/11

test-suite/squidconf/bad-acl-src-subnetwork-v6.conf.instructions

@@ -0,0 +1,2 @@
+skip-unless-autoconf-defines USE_IPV6 1
+expect-message WARNING:.*fee0::/11..is.a.subnetwork.of.*fec0::/10

test-suite/squidconf/bad-acl-src-subnetwork.conf

@@ -0,0 +1,8 @@
+## Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+##
+## Squid software is distributed under GPLv2+ license and includes
+## contributions from numerous individuals and organizations.
+## Please see the COPYING and CONTRIBUTORS files for details.
+##
+
+acl foo src 192.168.0.0/16 192.168.1.0/24

test-suite/squidconf/bad-acl-src-subnetwork.conf.instructions

@@ -0,0 +1 @@
+expect-message WARNING:.*192.168.1.0/24..is.a.subnetwork.of.*192.168.0.0/16

test-suite/test-squid-conf.sh

@@ -79,12 +79,7 @@ then
         then
             # Skip test unless the given macro is #defined in autoconf.h
             defineName=$p1
-
-            if test -n "$p2"
-            then
-                echo "$here: ERROR: Bad $instructionName instruction: Unexpected second parameter: $p2";
-                exit 1;
-            fi
+            defineValue=$p2
 
             autoconfHeader="$top_builddir/include/autoconf.h"
             if ! grep -q -w "$defineName" $autoconfHeader
@@ -99,6 +94,12 @@ then
                 exit 0;
             fi
 
+            if ! grep -q "# *define *\b$defineName\b $defineValue *$" $autoconfHeader
+            then
+                echo "$here: WARNING: Skipping $configFile test because $defineName is not $defineValue in $autoconfHeader";
+                exit 0;
+            fi
+
             if ! grep -q "# *define *\b$defineName\b" $autoconfHeader
             then
                 echo "$here: ERROR: Cannot determine status of $defineName macro";