]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Remove AclMatchedName from ACL::ParseAclLine() (#1642)
authorAlex Rousskov <rousskov@measurement-factory.com>
Sun, 28 Jan 2024 09:51:37 +0000 (09:51 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Mon, 29 Jan 2024 15:44:31 +0000 (15:44 +0000)
ACL parsing code needs to know the aclname parameter of the being-parsed
acl directive to report various errors. Most admins recognize their ACLs
by these names so reporting aclnames improves UX. Since before 1999
commit b6a2f15, Squid used a "temporary" and "ugly" trick that supplied
aclname via the unrelated global variable called AclMatchedName (which
has its own set of problems). Some ACL parsing code used AclMatchedName
in cache.log messages, but most ACL-related problems were still reported
without that information.

Passing ACL::name to each parsing-related function via an extra
parameter is not just ugly but impractical because some the low-level
parsing functions should not really know about ACLs. Instead, we reuse
existing CodeContext mechanism to report parsing context information (in
this case -- aclname).

We plan to enhance parsing context to cover directives other than "acl"
(without modifying every directive parser, of course), but this first
small step significantly reduces configuration code exposure to
AclMatchedName, unblocking ACL-related smooth reconfiguration
improvements.

12 files changed:
src/acl/Acl.cc
src/acl/Node.h
src/acl/ProtocolData.cc
src/acl/SplayInserter.h
src/base/CodeContext.h
src/external_acl.cc
test-suite/squidconf/bad-acl-dstdomain-dupe.conf
test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions
test-suite/squidconf/bad-acl-http-status-dupe.conf
test-suite/squidconf/bad-acl-http-status-dupe.conf.instructions
test-suite/squidconf/bad-acl-src-dupe.conf
test-suite/squidconf/bad-acl-src-dupe.conf.instructions

index 8cc5c5b3948b59ae6ec56171aa79c244e85aa143..d2e9c5faec40e74528230945470954e3a85221a1 100644 (file)
@@ -66,6 +66,22 @@ Make(TypeName typeName)
     return result;
 }
 
+/// CodeContext of the being-parsed acl directive
+class ParsingContext: public CodeContext
+{
+public:
+    using Pointer = RefCount<ParsingContext>;
+
+    explicit ParsingContext(const SBuf &name): name_(name) {}
+
+    /* CodeContext API */
+    ScopedId codeContextGist() const override;
+    std::ostream &detailCodeContext(std::ostream &os) const override;
+
+private:
+    SBuf name_; ///< the aclname parameter of the being-parsed acl directive
+};
+
 } // namespace Acl
 
 void
@@ -80,9 +96,7 @@ void
 Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey)
 {
     if (!newKey) {
-        throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName,
-                                   Debug::Extra, "ACL name: ", AclMatchedName),
-                            Here());
+        throw TextException(ToSBuf("An acl declaration is missing a ", keyParameterName), Here());
     }
 
     if (keyStorage.isEmpty()) {
@@ -96,12 +110,27 @@ Acl::SetKey(SBuf &keyStorage, const char *keyParameterName, const char *newKey)
     throw TextException(ToSBuf("Attempt to change the value of the ", keyParameterName, " argument in a subsequent acl declaration:",
                                Debug::Extra, "previously seen value: ", keyStorage,
                                Debug::Extra, "new/conflicting value: ", newKey,
-                               Debug::Extra, "ACL name: ", AclMatchedName,
                                Debug::Extra, "advice: Use a dedicated ACL name for each distinct ", keyParameterName,
                                " (and group those ACLs together using an 'any-of' ACL)."),
                         Here());
 }
 
+/* Acl::ParsingContext */
+
+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();
+}
+
+/* Acl::Node */
+
 void *
 Acl::Node::operator new (size_t)
 {
@@ -192,9 +221,6 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
 {
     /* we're already using strtok() to grok the line */
     char *t = nullptr;
-    Node *A = nullptr;
-    LOCAL_ARRAY(char, aclname, ACL_NAME_SZ);
-    int new_acl = 0;
 
     /* snarf the ACL name */
 
@@ -211,7 +237,16 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
         return;
     }
 
-    xstrncpy(aclname, t, ACL_NAME_SZ);
+    SBuf aclname(t);
+    CallParser(ParsingContext::Pointer::Make(aclname), [&] {
+        ParseNamed(parser, head, aclname.c_str()); // TODO: Convert Node::name to SBuf
+    });
+}
+
+/// parses acl directive parts that follow aclname
+void
+Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const char * const aclname)
+{
     /* snarf the ACL type */
     const char *theType;
 
@@ -252,6 +287,8 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
         theType = "client_connection_mark";
     }
 
+    Node *A = nullptr;
+    int new_acl = 0;
     if ((A = FindByName(aclname)) == nullptr) {
         debugs(28, 3, "aclParseAclLine: Creating ACL '" << aclname << "'");
         A = Acl::Make(theType);
@@ -268,22 +305,11 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
         new_acl = 0;
     }
 
-    /*
-     * Here we set AclMatchedName in case we need to use it in a
-     * warning message in Acl::SplayInserter::Merge().
-     */
-    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;
 
index 224a81e868c83117fd0749f3f26b03ae7735a9bf..747f76a5cfdce6e17ba00753ff84d91dafc6b2be 100644 (file)
@@ -90,6 +90,8 @@ private:
     /// \returns (linked) "line" Options supported by this Acl::Node
     /// \see Acl::Node::options()
     virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); }
+
+    static void ParseNamed(ConfigParser &, Node **head, const char *name);
 };
 
 } // namespace Acl
index e1ffbf1a5cb00339bb295cf39c55f37545752f6a..ca340cac4944fbb9cca26090fad10b9ccabc2ca1 100644 (file)
@@ -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
         }
     }
index 2871044ed8c4daabc3037ea3a7524fa4f8c60d23..bcfd8a8e95a789e6bce9ea4b5e56f21c22129ca3 100644 (file)
@@ -73,14 +73,14 @@ Acl::SplayInserter<DataValue>::Merge(Splay<Value> &storage, Value &&newItem)
 
         if (IsSubset(newItem, oldItem)) {
             debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Ignoring " << newItem << " because it is already covered by " << oldItem <<
-                   Debug::Extra << "advice: Remove value " << newItem << " from the ACL named " << AclMatchedName);
+                   Debug::Extra << "advice: Remove value " << newItem << " from the ACL");
             DestroyValue(newItem);
             return;
         }
 
         if (IsSubset(oldItem, newItem)) {
             debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Ignoring earlier " << oldItem << " because it is covered by " << newItem <<
-                   Debug::Extra << "advice: Remove value " << oldItem << " from the ACL named " << AclMatchedName);
+                   Debug::Extra << "advice: Remove value " << oldItem << " from the ACL");
             storage.remove(oldItem, comparator);
             DestroyValue(oldItem);
             continue; // still need to insert newItem (and it may conflict with other old items)
@@ -88,7 +88,7 @@ Acl::SplayInserter<DataValue>::Merge(Splay<Value> &storage, Value &&newItem)
 
         const auto combinedItem = MakeCombinedValue(oldItem, newItem);
         debugs(28, DBG_PARSE_NOTE(DBG_IMPORTANT), "WARNING: Merging overlapping " << newItem << " and " << oldItem << " into " << combinedItem <<
-               Debug::Extra << "advice: Replace values " << newItem << " and " << oldItem << " with " << combinedItem << " in the ACL named " << AclMatchedName);
+               Debug::Extra << "advice: Replace values " << newItem << " and " << oldItem << " with " << combinedItem << " in the ACL");
         DestroyValue(newItem);
         newItem = combinedItem;
         storage.remove(oldItem, comparator);
index ee9094bf4082ea0344bbec35e883b83e5601201f..cfda2a77a462a8973045f9ef8249c634aec865f8 100644 (file)
@@ -106,20 +106,38 @@ public:
     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).
-///
+/// A helper that calls the given function in the given call context. If the
+/// function throws, the call context is preserved, so that the exception is
+/// associated with the context that triggered it.
+template <typename Fun>
+inline void
+CallAndRestore_(const CodeContext::Pointer &context, Fun &&fun)
+{
+    const auto savedCodeContext(CodeContext::Current());
+    CodeContext::Reset(context);
+    fun();
+    CodeContext::Reset(savedCodeContext);
+}
+
 /// Service code running in its own service context should use this function.
+/// \sa CallAndRestore_()
 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);
+}
+
+/// To supply error-reporting code with parsing context X (where the error
+/// occurred), parsing code should use this function when initiating parsing
+/// inside that context X.
+/// \sa CallAndRestore_()
+template <typename Fun>
+inline void
+CallParser(const CodeContext::Pointer &parsingContext, Fun &&parse)
+{
+    CallAndRestore_(parsingContext, parse);
 }
 
 /// Executes `service` in `serviceContext` but due to automatic caller context
index 7238adbc8d69b9b3ccdd09eae27d2bee4cda80d0..a64155baf43abe297b775d6f85698f78b7d2854f 100644 (file)
@@ -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);
index 11ead73a6930bf43c33a6814a82cbc32daa56aab..c7506f8c71c5320ea68b18cf19120d19855e3500 100644 (file)
@@ -6,7 +6,7 @@
 ##
 
 acl test11 dstdomain .example.com example.com
-acl test12 dstdomain example.com .example.com
+acl test12 dstdomain example.org .example.org
 
 acl test21 dstdomain .example.com a.example.com
 acl test22 dstdomain b.example.com .example.com
index 868234ee655d196e7c37f5f8665555f03831935d..ff18d9c66a7574f46fa07b662cf302fd37c3feab 100644 (file)
@@ -1,25 +1,31 @@
 expect-messages <<END
 WARNING: Ignoring example.com because it is already covered by .example.com
-    advice: Remove value example.com from the ACL named test11
+    advice: Remove value example.com from the ACL
+    acl name: test11
 
-WARNING: Ignoring earlier example.com because it is covered by .example.com
-    advice: Remove value example.com from the ACL named test12
+WARNING: Ignoring earlier example.org because it is covered by .example.org
+    advice: Remove value example.org from the ACL
+    acl name: test12
 
 WARNING: Ignoring a.example.com because it is already covered by .example.com
-    advice: Remove value a.example.com from the ACL named test21
+    advice: Remove value a.example.com from the ACL
+    acl name: test21
 
 WARNING: Ignoring earlier b.example.com because it is covered by .example.com
-    advice: Remove value b.example.com from the ACL named test22
+    advice: Remove value b.example.com from the ACL
+    acl name: test22
 
 WARNING: Ignoring .example.com because it is already covered by .example.com
-    advice: Remove value .example.com from the ACL named test31
-
+    advice: Remove value .example.com from the ACL
 WARNING: Ignoring c.example.com because it is already covered by .example.com
-    advice: Remove value c.example.com from the ACL named test31
+    advice: Remove value c.example.com from the ACL
+    acl name: test31
 
 WARNING: Ignoring earlier .d.example.com because it is covered by .example.com
-    advice: Remove value .d.example.com from the ACL named test41
+    advice: Remove value .d.example.com from the ACL
+    acl name: test41
 
 WARNING: Ignoring .e.example.com because it is already covered by .example.com
-    advice: Remove value .e.example.com from the ACL named test42
+    advice: Remove value .e.example.com from the ACL
+    acl name: test42
 END
index fdb392545ed1a219492e753b57ae81bc8ffb3de0..475345d4de126992267a7846fe2c508668233ce9 100644 (file)
@@ -15,4 +15,4 @@ acl test14 http_status 304
 acl test21 http_status 100-300 350 200-400
 acl test22 http_status 150 200-400 100-300
 
-acl test30 http_status 300 400 300-400
+acl test30 http_status 300 399 300-399
index d043181eb43e502230d62034cecdbbb87aca991b..56422ac75d6f5963b2413069c2665525d4fdbf05 100644 (file)
@@ -1,28 +1,35 @@
 expect-messages <<END
 WARNING: Ignoring 400 because it is already covered by 400
-    advice: Remove value 400 from the ACL named test11
+    advice: Remove value 400 from the ACL
+    acl name: test11
 
 WARNING: Ignoring 407 because it is already covered by 407
-    advice: Remove value 407 from the ACL named test12
+    advice: Remove value 407 from the ACL
+    acl name: test12
 
 WARNING: Ignoring earlier 200 because it is covered by 200-300
-    advice: Remove value 200 from the ACL named test13
+    advice: Remove value 200 from the ACL
+    acl name: test13
 
 WARNING: Ignoring 304 because it is already covered by 300-399
-    advice: Remove value 304 from the ACL named test14
+    advice: Remove value 304 from the ACL
+    acl name: test14
 
 WARNING: Ignoring earlier 350 because it is covered by 200-400
-    advice: Remove value 350 from the ACL named test21
+    advice: Remove value 350 from the ACL
 WARNING: Merging overlapping 200-400 and 100-300 into 100-400
-    advice: Replace values 200-400 and 100-300 with 100-400 in the ACL named test21
+    advice: Replace values 200-400 and 100-300 with 100-400 in the ACL
+    acl name: test21
 
 WARNING: Merging overlapping 100-300 and 200-400 into 100-400
-    advice: Replace values 100-300 and 200-400 with 100-400 in the ACL named test22
+    advice: Replace values 100-300 and 200-400 with 100-400 in the ACL
 WARNING: Ignoring earlier 150 because it is covered by 100-400
-    advice: Remove value 150 from the ACL named test22
+    advice: Remove value 150 from the ACL
+    acl name: test22
 
-WARNING: Ignoring earlier 400 because it is covered by 300-400
-    advice: Remove value 400 from the ACL named test30
-WARNING: Ignoring earlier 300 because it is covered by 300-400
-    advice: Remove value 300 from the ACL named test30
+WARNING: Ignoring earlier 399 because it is covered by 300-399
+    advice: Remove value 399 from the ACL
+WARNING: Ignoring earlier 300 because it is covered by 300-399
+    advice: Remove value 300 from the ACL
+    acl name: test30
 END
index 0bfdf3d751ba91704488f95d7c69fbefb03938a7..4162883e77ce5afd0748ac779e972dc60a34d864 100644 (file)
@@ -8,7 +8,7 @@
 acl test11 src 127.0.0.1  127.0.0.0-127.0.0.255
 acl test12 src 192.168.1.0/24 192.168.0.0/16
 
-acl test13 src 127.0.0.0-127.0.0.255  127.0.0.1
+acl test13 src 127.0.0.0-127.0.0.255  127.0.0.2
 acl test14 src 127.0.0.0-127.0.0.128  127.0.0.128-127.0.0.255
 
 acl test15 src 10.0.0.1-10.0.0.128 10.0.0.0-10.0.0.1 10.0.0.128-10.0.0.255
@@ -16,7 +16,7 @@ acl test15 src 10.0.0.1-10.0.0.128 10.0.0.0-10.0.0.1 10.0.0.128-10.0.0.255
 acl test25 dst 127.0.0.0-127.0.0.128/32  127.0.0.128-127.1.0.255
 
 acl test36 dst 127.0.0.1-127.0.0.128  127.0.0.0-127.1.0.0/16
-acl test37 dst 127.0.0.0-127.1.0.0/16 127.0.0.1-127.0.0.128
+acl test37 dst 127.1.0.0-127.2.0.0/16 127.1.0.1-127.1.0.128
 
 # TODO: make configurable depending on USE_IPV6
 # acl test41 src bad::1 bad::0-bad::f
index 0c4d7b1b9e02ff5f51e24568b1afdbb9d37bf3df..717b1eae8b036471505250916e29e2830ea1cf2f 100644 (file)
@@ -1,40 +1,52 @@
 expect-messages <<END
 WARNING: Ignoring earlier 127.0.0.1 because it is covered by 127.0.0.0-127.0.0.255
-    advice: Remove value 127.0.0.1 from the ACL named test11
+    advice: Remove value 127.0.0.1 from the ACL
+    acl name: test11
 
 WARNING: Ignoring earlier 192.168.1.0/24 because it is covered by 192.168.0.0/16
-    advice: Remove value 192.168.1.0/24 from the ACL named test12
+    advice: Remove value 192.168.1.0/24 from the ACL
+    acl name: test12
 
-WARNING: Ignoring 127.0.0.1 because it is already covered by 127.0.0.0-127.0.0.255
-    advice: Remove value 127.0.0.1 from the ACL named test13
+WARNING: Ignoring 127.0.0.2 because it is already covered by 127.0.0.0-127.0.0.255
+    advice: Remove value 127.0.0.2 from the ACL
+    acl name: test13
 
 WARNING: Merging overlapping 127.0.0.128-127.0.0.255 and 127.0.0.0-127.0.0.128 into 127.0.0.0-127.0.0.255
-    advice: Replace values 127.0.0.128-127.0.0.255 and 127.0.0.0-127.0.0.128 with 127.0.0.0-127.0.0.255 in the ACL named test14
+    advice: Replace values 127.0.0.128-127.0.0.255 and 127.0.0.0-127.0.0.128 with 127.0.0.0-127.0.0.255 in the ACL
+    acl name: test14
 
 WARNING: Merging overlapping 10.0.0.0-10.0.0.1 and 10.0.0.1-10.0.0.128 into 10.0.0.0-10.0.0.128
-    advice: Replace values 10.0.0.0-10.0.0.1 and 10.0.0.1-10.0.0.128 with 10.0.0.0-10.0.0.128 in the ACL named test15
+    advice: Replace values 10.0.0.0-10.0.0.1 and 10.0.0.1-10.0.0.128 with 10.0.0.0-10.0.0.128 in the ACL
 WARNING: Merging overlapping 10.0.0.128-10.0.0.255 and 10.0.0.0-10.0.0.128 into 10.0.0.0-10.0.0.255
-    advice: Replace values 10.0.0.128-10.0.0.255 and 10.0.0.0-10.0.0.128 with 10.0.0.0-10.0.0.255 in the ACL named test15
+    advice: Replace values 10.0.0.128-10.0.0.255 and 10.0.0.0-10.0.0.128 with 10.0.0.0-10.0.0.255 in the ACL
+    acl name: test15
 
 WARNING: Merging overlapping 127.0.0.128-127.1.0.255 and 127.0.0.0-127.0.0.128 into 127.0.0.0-127.1.0.255
-    advice: Replace values 127.0.0.128-127.1.0.255 and 127.0.0.0-127.0.0.128 with 127.0.0.0-127.1.0.255 in the ACL named test25
+    advice: Replace values 127.0.0.128-127.1.0.255 and 127.0.0.0-127.0.0.128 with 127.0.0.0-127.1.0.255 in the ACL
+    acl name: test25
 
 WARNING: Ignoring earlier 127.0.0.1-127.0.0.128 because it is covered by 127.0.0.0-127.1.0.0/16
-    advice: Remove value 127.0.0.1-127.0.0.128 from the ACL named test36
+    advice: Remove value 127.0.0.1-127.0.0.128 from the ACL
+    acl name: test36
 
-WARNING: Ignoring 127.0.0.1-127.0.0.128 because it is already covered by 127.0.0.0-127.1.0.0/16
-    advice: Remove value 127.0.0.1-127.0.0.128 from the ACL named test37
+WARNING: Ignoring 127.1.0.1-127.1.0.128 because it is already covered by 127.1.0.0-127.2.0.0/16
+    advice: Remove value 127.1.0.1-127.1.0.128 from the ACL
+    acl name: test37
 END
 
 # TODO: skip-unless-autoconf-defines USE_IPV6 1
 # WARNING: Ignoring earlier bad::1 because it is covered by bad::-bad::f
-#     advice: Remove value bad::1 from the ACL named test41
+#     advice: Remove value bad::1 from the ACL
+#     acl name: test41
 #
 # WARNING: Ignoring dead:: because it is already covered by dead::-dead::
-#     advice: Remove value dead:: from the ACL named test42
+#     advice: Remove value dead:: from the ACL
+#     acl name: test42
 #
 # WARNING: Ignoring bad:: because it is already covered by bad::/64
-#     advice: Remove value bad:: from the ACL named test43
+#     advice: Remove value bad:: from the ACL
+#     acl name: test43
 #
 # WARNING: Ignoring beef:bad::/64 because it is already covered by beef::/16
-#     advice: Remove value beef:bad::/64 from the ACL named test44
+#     advice: Remove value beef:bad::/64 from the ACL
+#     acl name: test44