From: Alex Rousskov Date: Thu, 11 Jan 2024 07:10:15 +0000 (+0000) Subject: Fix dupe handling in Splay ACLs: src, dst, http_status, etc. (#1632) X-Git-Tag: SQUID_7_0_1~240 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=0898d0f4c23df6854c035919c5480e87f62b5784;p=thirdparty%2Fsquid.git Fix dupe handling in Splay ACLs: src, dst, http_status, etc. (#1632) Squid was dangerously mishandling duplicate[^1] values in ACLs with Splay tree storage: src, dst, localip, http_status, dstdomain, srcdomain, and ssl::server_name. These problems were all rooted in the same old code but had diverged across two ACL groups, as detailed in the corresponding sections below. Fortunately, all known problems were accompanied with Squid cache.log WARNINGs: Squid configurations that do not emit cited or similar warnings are not affected by these bugs. [^1]: Squid Splay trees can only store unique values. Uniqueness is defined in terms of a configurable comparison function that returns zero when the two values are "not unique" (i.e. are considered to be "duplicated" or "equal" in that Splay context). Those two "equal" values may differ a lot in other contexts! For example, the following two status code ranges are equal from acl_httpstatus_data::compare() point of view, but are obviously very different from an admin and http_access rules points of view: 200-200 and 200-299. ### Group 1: src, dst, localip, and http_status ACLs These ACLs ignored (i.e. never matched) some configured ACL values in problematic use cases. They also gave wrong "remove X" advice and incorrectly classified values as being subranges or subsets: Processing: acl testA http_status 200 200-300 WARNING: '200-300' is a subrange of '200' WARNING: because of this '200-300' is ignored ... Processing: acl testB http_status 300-400 200-300 WARNING: '200-300' is a subrange of '300-400' WARNING: because of this '200-300' is ignored ... WARNING: You should probably remove '300-400' from the ACL... Processing: acl testC src 10.0.0.1 10.0.0.0-10.0.0.255 WARNING: (B) 10.0.0.1 is a subnetwork of (A) 10.0.0.0-10.0.0.255 WARNING: because of this 10.0.0.0-10.0.0.255 is ignored... Processing: acl testD src 10.0.0.0-10.0.0.1 10.0.0.1-10.0.0.255 WARNING: (A) 10.0.0.1-10.0.0.255 is a subnetwork of (B) 10.0.0.0-... WARNING: because of this 10.0.0.1-10.0.0.255 is ignored... WARNING: You should probably remove 10.0.0.1-10.0.0.255 from the ACL Since 2002 commit 96da6e8, IP-based ACLs like src, dst, and localip eliminate duplicates[^1] among configured ACL values. That elimination code was buggy since inception. Those bugs were later duplicated in http_status code (2005 commit a0ec9f6). This change fixes those bugs. To correctly eliminate duplicates when facing two (fully or partially) overlapping ranges -- new A and old B -- we must pick the right corrective action depending on the kind of overlap: * A is a subrange of B: Ignore fully duplicated new range A. Keep B. * B is a subrange of A: Remove fully duplicated old range B. Add A. * A partially overlaps with B: Add a union of A and B. Remove B. Both acl_httpstatus_data::compare() and acl_ip_data::NetworkCompare() mishandled the last two cases because these functions effectively implemented the following buggy logic instead: - in all three cases: Ignore new range A. Keep B. Their WARNINGs also suggested wrong corrective actions in two mishandled cases (see the last WARNING lines in testB and testD output above). ### Group 2: dstdomain, srcdomain, and ssl::server_name ACLs Processing: acl testE dstdomain .example.com example.com ERROR: '.example.com' is a subdomain of 'example.com' ERROR: You need to remove '.example.com' from the ACL named 'testE' 2002 commit 96da6e8 bugs mentioned in Group 1 section stopped affecting domain-based ACLs (i.e. dstdomain, srcdomain, and ssl::server_name) in 2011 commit 14e563c that adjusted aclDomainCompare() to self_destruct() in problematic cases. However, that adjustment emitted wrong advice and incorrect subdomain classification in cases where strlen() checks do not work for determining which of the two configured ACL values should be removed (see testE ERRORs above). This change improves that handling by replacing the call to self_destruct() with proper duplicate[^1] resolution code (and fixing cache.log messages). We (now) support duplicate values instead of rejecting configurations containing them because duplicate values do not invalidate an ACL -- an ACL with duplicates could match as expected. It may be difficult for some admins to avoid duplication, especially when ACL values come from multiple sources. Squid should continue to warn about duplicates (because they waste resources and may indicate a deeper problem), but killing Squid or otherwise rejecting ACLs with duplicates is bad UX. N.B. Domain-based ACLs use sets of values rather than "ranges" discussed in Group 1 section, but domain sets follow the same basic principles. ### All of the above seven ACLs The problems in both ACL groups were fixed by factoring out "insert ACL values while correctly handling duplicates" algorithm (see the three bullets in Group 1 section) into Acl::SplayInserter::Merge() function. Without duplicates, the new ACL value insertion code has the same cost as the old one. The vast majority of duplicate cases incur a constant additional overhead because Splay tree dynamic reorganization makes the right tree nodes immediately available. A few cases duplicate double the number of comparisons during tree searches (when Splay reorganization cannot cope with a particularly "bad" ACL value order), but since these "bad" cases ought to be very rare, and since all problematic cases are accompanied by WARNINGs, this extra cost is deemed acceptable. This change also fixes memory leaks associated with ignored ACL values. Also added test-suite/test-squid-conf.sh support for matching multiple stderr messages. Without this, testing a variety of closely-related cases requires creating lots of test configuration files (two per test case), increasing noise and, more importantly, making it difficult to handle related test cases as one coherent collection. The new "expect-messages" feature is barely sufficient for testing these changes, but we are now at (or perhaps even beyond) the limit of what can be reasonably done using shell scripts and test instruction files: The next step is to convert instruction files (and likely some test cases themselves!) to scripts written in Perl or a better language. --- diff --git a/include/splay.h b/include/splay.h index 5a113261e9..30f11cc27d 100644 --- a/include/splay.h +++ b/include/splay.h @@ -61,7 +61,9 @@ public: template Value const *find (FindValue const &, int( * compare)(FindValue const &a, Value const &b)) const; - void insert(Value const &, SPLAYCMP *compare); + /// If the given value matches a stored one, returns that matching value. + /// Otherwise, stores the given unique value and returns nil. + const Value *insert(const Value &, SPLAYCMP *); void remove(Value const &, SPLAYCMP *compare); @@ -314,17 +316,19 @@ Splay::find (FindValue const &value, int( * compare)(FindValue const &a, Valu } template -void +typename Splay::Value const * Splay::insert(Value const &value, SPLAYCMP *compare) { - if (find(value, compare) != nullptr) // ignore duplicates - return; + if (const auto similarValue = find(value, compare)) + return similarValue; // do not insert duplicates if (head == nullptr) head = new SplayNode(value); else head = head->insert(value, compare); ++elements; + + return nullptr; // no duplicates found } template diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc index 376f252154..489eab53b8 100644 --- a/src/acl/Acl.cc +++ b/src/acl/Acl.cc @@ -271,7 +271,7 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head) /* * Here we set AclMatchedName in case we need to use it in a - * warning message in aclDomainCompare(). + * warning message in Acl::SplayInserter::Merge(). */ AclMatchedName = A->name; /* ugly */ diff --git a/src/acl/DomainData.cc b/src/acl/DomainData.cc index cb5fd89951..1124989a09 100644 --- a/src/acl/DomainData.cc +++ b/src/acl/DomainData.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "acl/Checklist.h" #include "acl/DomainData.h" +#include "acl/SplayInserter.h" #include "anyp/Uri.h" #include "cache_cf.h" #include "ConfigParser.h" @@ -59,48 +60,6 @@ aclHostDomainCompare( char *const &a, char * const &b) return matchDomainName(h, d); } -/* compare two domains */ - -template -int -aclDomainCompare(T const &a, T const &b) -{ - char * const d1 = static_cast(b); - char * const d2 = static_cast(a); - int ret; - ret = aclHostDomainCompare(d1, d2); - - if (ret != 0) { - char *const d3 = d2; - char *const d4 = d1; - ret = aclHostDomainCompare(d3, d4); - if (ret == 0) { - // 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 << "'"); - 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 << "'"); - 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(); - } - - return ret; -} - bool ACLDomainData::match(char const *host) { @@ -132,6 +91,70 @@ ACLDomainData::dump() const return visitor.contents; } +template <> +int +Acl::SplayInserter::Compare(const Value &a, const Value &b) +{ + // If X represents a set of matching domain names (e.g., .example.com), then + // matchDomainName(X, Y) uses a single domain name from X by removing the + // leading dot (e.g., example.com). We call that name "the root of X". If X + // is a single domain name, then its root is X itself. Since domain sets + // cannot have _partial_ overlaps (unlike IP or integer ranges), testing + // roots is enough to detect duplicates and establish correct set order. + + if (matchDomainName(b, a)) { + // Set A does not contain B's root. If set B contains A's root, then the + // call below will return 0, signaling duplicates. Otherwise, A and B + // have no common values, and the call below will correctly order the + // two sets, mimicking the order used by the Splay comparison function + // in match(). + return matchDomainName(a, b); + } else { + // Signal duplicates because set A contains B's root (at least). + return 0; + } +} + +template <> +bool +Acl::SplayInserter::IsSubset(const Value &a, const Value &b) +{ + // A value that starts with a dot matches a set of the corresponding domain + // names. Other values are individual domain names that match themselves. + // \sa matchDomainName() + + if (*a == '.' && *b == '.') { + // A and B are overlapping sets. More characters imply a smaller set. + return strlen(a) >= strlen(b); + } + + if (*a != '.' && *b != '.') { + // A and B are identical individual domain names + return true; + } + + // Either A or B is a set. The other one is a domain name inside that set. + // That domain name may use fewer or more characters (e.g., both example.com + // and x.example.com domains belong to the same .example.com set), so we + // cannot use a strlen()-based test here. + return *b == '.'; +} + +template <> +Acl::SplayInserter::Value +Acl::SplayInserter::MakeCombinedValue(const Value &, const Value &) +{ + Assure(!"domain name sets cannot partially overlap"); + return nullptr; // unreachable code +} + +template <> +void +Acl::SplayInserter::DestroyValue(Value v) +{ + xfree(v); +} + void ACLDomainData::parse() { @@ -140,7 +163,7 @@ ACLDomainData::parse() while (char *t = ConfigParser::strtokFile()) { Tolower(t); - domains->insert(xstrdup(t), aclDomainCompare); + Acl::SplayInserter::Merge(*domains, xstrdup(t)); } } diff --git a/src/acl/HttpStatus.cc b/src/acl/HttpStatus.cc index 1cda99ecf3..14e21341df 100644 --- a/src/acl/HttpStatus.cc +++ b/src/acl/HttpStatus.cc @@ -11,9 +11,11 @@ #include "squid.h" #include "acl/FilledChecklist.h" #include "acl/HttpStatus.h" +#include "acl/SplayInserter.h" #include "debug/Stream.h" #include "HttpReply.h" +#include #include static void aclParseHTTPStatusList(Splay **curlist); @@ -37,23 +39,36 @@ acl_httpstatus_data::toStr() const return rv; } -int acl_httpstatus_data::compare(acl_httpstatus_data* const& a, acl_httpstatus_data* const& b) +template <> +int +Acl::SplayInserter::Compare(const Value &a, const Value &b) { - int ret; - ret = aclHTTPStatusCompare(b, a); - - if (ret != 0) - ret = aclHTTPStatusCompare(a, b); - - 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 << "'"); - } + return aclHTTPStatusCompare(a, b); +} - return ret; +template <> +bool +Acl::SplayInserter::IsSubset(const Value &a, const Value &b) +{ + return b->status1 <= a->status1 && a->status2 <= b->status2; +} + +template <> +Acl::SplayInserter::Value +Acl::SplayInserter::MakeCombinedValue(const Value &a, const Value &b) +{ + const auto minLeft = std::min(a->status1, b->status1); + const auto maxRight = std::max(a->status2, b->status2); + return new acl_httpstatus_data(minLeft, maxRight); +} + +/// reports acl_httpstatus_data using squid.conf http_status ACL value format +static std::ostream & +operator <<(std::ostream &os, const acl_httpstatus_data *status) +{ + if (status) + os << status->toStr(); + return os; } ACLHTTPStatus::ACLHTTPStatus (char const *theClass) : data(nullptr), class_ (theClass) @@ -109,7 +124,7 @@ aclParseHTTPStatusList(Splay **curlist) { while (char *t = ConfigParser::strtokFile()) { if (acl_httpstatus_data *q = aclParseHTTPStatusData(t)) - (*curlist)->insert(q, acl_httpstatus_data::compare); + Acl::SplayInserter::Merge(**curlist, std::move(q)); } } @@ -132,13 +147,13 @@ aclMatchHTTPStatus(Splay **dataptr, const Http::StatusCode static int aclHTTPStatusCompare(acl_httpstatus_data * const &a, acl_httpstatus_data * const &b) { - if (a->status1 < b->status1) - return 1; + if (a->status2 < b->status1) + return 1; // the entire range a is to the left of range b if (a->status1 > b->status2) - return -1; + return -1; // the entire range a is to the right of range b - return 0; + return 0; // equal or partially overlapping ranges } struct HttpStatusAclDumpVisitor { diff --git a/src/acl/HttpStatus.h b/src/acl/HttpStatus.h index 6e9997e540..86d1952a93 100644 --- a/src/acl/HttpStatus.h +++ b/src/acl/HttpStatus.h @@ -19,8 +19,6 @@ struct acl_httpstatus_data { acl_httpstatus_data(int); acl_httpstatus_data(int, int); SBuf toStr() const; // was toStr - - static int compare(acl_httpstatus_data* const& a, acl_httpstatus_data* const& b); }; /// \ingroup ACLAPI diff --git a/src/acl/Ip.cc b/src/acl/Ip.cc index 657b817f35..b4a2d8e314 100644 --- a/src/acl/Ip.cc +++ b/src/acl/Ip.cc @@ -11,6 +11,7 @@ #include "squid.h" #include "acl/Checklist.h" #include "acl/Ip.h" +#include "acl/SplayInserter.h" #include "cache_cf.h" #include "ConfigParser.h" #include "debug/Stream.h" @@ -18,6 +19,8 @@ #include "MemBuf.h" #include "wordlist.h" +#include + void * ACLIP::operator new (size_t) { @@ -77,6 +80,62 @@ acl_ip_data::toSBuf() const return SBuf(tmpbuf); } +Ip::Address +acl_ip_data::firstAddress() const +{ + auto ip = addr1; + if (!mask.isNoAddr()) + ip.applyMask(mask); + return ip; +} + +Ip::Address +acl_ip_data::lastAddress() const +{ + auto ip = addr2.isAnyAddr() ? addr1 : addr2; + if (!mask.isNoAddr()) + ip.turnMaskedBitsOn(mask); + return ip; +} + +template <> +int +Acl::SplayInserter::Compare(const Value &a, const Value &b) +{ + if (a->lastAddress() < b->firstAddress()) + return -1; // the entire range a is to the left of range b + + if (a->firstAddress() > b->lastAddress()) + return +1; // the entire range a is to the right of range b + + return 0; // equal or partially overlapping ranges +} + +template <> +bool +Acl::SplayInserter::IsSubset(const Value &a, const Value &b) +{ + return b->firstAddress() <= a->firstAddress() && a->lastAddress() <= b->lastAddress(); +} + +template <> +Acl::SplayInserter::Value +Acl::SplayInserter::MakeCombinedValue(const Value &a, const Value &b) +{ + const auto minLeft = std::min(a->firstAddress(), b->firstAddress()); + const auto maxRight = std::max(a->lastAddress(), b->lastAddress()); + return new acl_ip_data(minLeft, maxRight, Ip::Address::NoAddr(), nullptr); +} + +/// reports acl_ip_data using squid.conf ACL value format +static std::ostream & +operator <<(std::ostream &os, acl_ip_data *value) +{ + if (value) + os << value->toSBuf(); + return os; +} + /* * aclIpAddrNetworkCompare - The guts of the comparison for IP ACLs * matching checks. The first argument (p) is a "host" address, @@ -108,45 +167,6 @@ aclIpAddrNetworkCompare(acl_ip_data * const &p, acl_ip_data * const &q) } } -/* - * acl_ip_data::NetworkCompare - Compare two acl_ip_data entries. Strictly - * used by the splay insertion routine. It emits a warning if it - * detects a "collision" or overlap that would confuse the splay - * sorting algorithm. Much like aclDomainCompare. - * The first argument (p) is a "host" address, i.e. the IP address of a cache client. - * The second argument (b) is a "network" address that might have a subnet and/or range. - * We mask the host address bits with the network subnet mask. - */ -int -acl_ip_data::NetworkCompare(acl_ip_data * const & a, acl_ip_data * const &b) -{ - int ret; - bool bina = true; - ret = aclIpAddrNetworkCompare(b, a); - - if (ret != 0) { - bina = false; - ret = aclIpAddrNetworkCompare(a, b); - } - - if (ret == 0) { - char buf_n1[3*(MAX_IPSTRLEN+1)]; - char buf_n2[3*(MAX_IPSTRLEN+1)]; - if (bina) { - b->toStr(buf_n1, 3*(MAX_IPSTRLEN+1)); - a->toStr(buf_n2, 3*(MAX_IPSTRLEN+1)); - } else { - 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 << "'"); - } - - return ret; -} - /** * Decode an ascii representation (asc) of a IP netmask address or CIDR, * and place resulting information in mask. @@ -367,6 +387,20 @@ acl_ip_data::FactoryParse(const char *t) if (changed) debugs(28, DBG_CRITICAL, "WARNING: aclIpParseIpData: Netmask masks away part of the specified IP in '" << t << "'"); + // TODO: Either switch match() to Acl::SplayInserter::Compare() + // range logic (that does not have these problems) OR warn that some (or + // even all) addresses will never match this configured ACL value when + // `q->addr1.applyMask()` above is positive: + // + // * A single configured IP value will never match: + // A.matchIPAddr(q->addr1) in aclIpAddrNetworkCompare() will not return 0. + // For example, `acl x src 127.0.0.1/24` does not match any address. + // + // * A configured IP range will not match any q->addr1/mask IPs: + // (A >= q->addr1) in aclIpAddrNetworkCompare() is false and + // A.matchIPAddr(q->addr1) will not return 0. + // For example, `acl y src 10.0.0.1-10.0.0.255/24` does not match 10.0.0.1. + debugs(28,9, "Parsed: " << q->addr1 << "-" << q->addr2 << "/" << q->mask << "(/" << q->mask.cidr() <<")"); /* 1.2.3.4/255.255.255.0 --> 1.2.3.0 */ @@ -438,8 +472,7 @@ ACLIP::parse() /* pop each result off the list and add it to the data tree individually */ acl_ip_data *next_node = q->next; q->next = nullptr; - if (!data->find(q,acl_ip_data::NetworkCompare)) - data->insert(q, acl_ip_data::NetworkCompare); + Acl::SplayInserter::Merge(*data, std::move(q)); q = next_node; } } diff --git a/src/acl/Ip.h b/src/acl/Ip.h index 10dd7ce36f..3356402c94 100644 --- a/src/acl/Ip.h +++ b/src/acl/Ip.h @@ -20,7 +20,6 @@ class acl_ip_data public: static acl_ip_data *FactoryParse(char const *); - static int NetworkCompare(acl_ip_data * const & a, acl_ip_data * const &b); acl_ip_data (); @@ -28,6 +27,12 @@ public: void toStr(char *buf, int len) const; SBuf toSBuf() const; + /// minimum (masked) address that matches this configured ACL value + Ip::Address firstAddress() const; + + /// maximum (masked) address that matches this configured ACL value + Ip::Address lastAddress() const; + Ip::Address addr1; Ip::Address addr2; diff --git a/src/acl/Makefile.am b/src/acl/Makefile.am index 6e188ffc65..4dd16ce4da 100644 --- a/src/acl/Makefile.am +++ b/src/acl/Makefile.am @@ -24,6 +24,7 @@ libapi_la_SOURCES = \ InnerNode.h \ Options.cc \ Options.h \ + SplayInserter.h \ Tree.cc \ Tree.h \ forward.h diff --git a/src/acl/SplayInserter.h b/src/acl/SplayInserter.h new file mode 100644 index 0000000000..2871044ed8 --- /dev/null +++ b/src/acl/SplayInserter.h @@ -0,0 +1,101 @@ +/* + * 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. + */ + +#ifndef SQUID_SRC_ACL_SPLAYINSERTER_H +#define SQUID_SRC_ACL_SPLAYINSERTER_H + +#include "acl/Acl.h" +#include "debug/Stream.h" +#include "globals.h" +#include "splay.h" + +namespace Acl { + +/// Helps populate a Splay tree with configured ACL parameter values and their +/// duplicate-handling derivatives (each represented by a DataValue object). +template +class SplayInserter +{ +public: + using Value = DataValue; + + /// If necessary, updates the splay tree to match all individual values that + /// match the given parsed ACL parameter value. If the given value itself is + /// not added to the tree (e.g., because it is a duplicate), it is destroyed + /// using DestroyValue(). Otherwise, the given value will be destroyed + /// later, during subsequent calls to this method or free_acl(). + static void Merge(Splay &, Value &&); + +private: + /// SplayInserter users are expected to specialize all or most of the static + /// methods below. Most of these methods have no generic implementation. + + /// A Splay::SPLAYCMP function for comparing parsed ACL parameter values. + /// This function must work correctly with all valid ACL parameter values, + /// including those representing sets or ranges. The order specified by this + /// function must be the same as the order specified by the SPLAYCMP + /// function used later by ACL::match(). + /// \retval -1 when a < b (this function defines what "less" means in Merge() context) + /// \retval +1 when b < a + /// \retval 0 all other cases (i.e. when a and b overlap) + /// Here, two values overlap if they are identical, if one contains all + /// values from another, or if one contains at least one value from another. + static int Compare(const Value &a, const Value &b); + + /// whether the set of values matched by `a` contains the entire set of + /// values matched by `b`, including cases where `a` is identical to `b` + /// \prec The two values overlap: Compare(a, b) == 0 + static bool IsSubset(const Value &a, const Value &b); + + /// Creates a new Value that matches all individual values matched by `a` + /// and all individual values matched by `b` but no other values. + /// \prec The two values overlap: Compare(a, b) == 0 + static Value MakeCombinedValue(const Value &a, const Value &b); + + /// A Splay::SPLAYFREE-like function that destroys parsed ACL parameter values. + static void DestroyValue(Value v) { delete v; } +}; + +} // namespace Acl + +template +void +Acl::SplayInserter::Merge(Splay &storage, Value &&newItem) +{ + const auto comparator = &SplayInserter::Compare; + while (const auto oldItemPointer = storage.insert(newItem, comparator)) { + const auto oldItem = *oldItemPointer; + + 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); + 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); + storage.remove(oldItem, comparator); + DestroyValue(oldItem); + continue; // still need to insert newItem (and it may conflict with other old items) + } + + 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); + DestroyValue(newItem); + newItem = combinedItem; + storage.remove(oldItem, comparator); + DestroyValue(oldItem); + continue; // still need to insert updated newItem (and it may conflict with other old items) + } +} + +#endif /* SQUID_SRC_ACL_SPLAYINSERTER_H */ + diff --git a/src/ip/Address.cc b/src/ip/Address.cc index 8022b46f20..62a07cfb9c 100644 --- a/src/ip/Address.cc +++ b/src/ip/Address.cc @@ -101,6 +101,16 @@ Ip::Address::applyMask(Ip::Address const &mask_addr) return changes; } +void +Ip::Address::turnMaskedBitsOn(const Address &mask) +{ + const auto addressWords = reinterpret_cast(&mSocketAddr_.sin6_addr); + const auto maskWords = reinterpret_cast(&mask.mSocketAddr_.sin6_addr); + const auto len = sizeof(mSocketAddr_.sin6_addr)/sizeof(uint32_t); + for (size_t i = 0; i < len; ++i) + addressWords[i] |= ~maskWords[i]; +} + void Ip::Address::applyClientMask(const Address &mask) { diff --git a/src/ip/Address.h b/src/ip/Address.h index b3ccf24322..9f4822e565 100644 --- a/src/ip/Address.h +++ b/src/ip/Address.h @@ -194,6 +194,11 @@ public: /// IPv6 clients use 'privacy addressing' instead. void applyClientMask(const Address &mask); + /// Set bits of the stored IP address on if they are on in the given mask. + /// For example, supplying a /24 mask turns 127.0.0.1 into 127.0.0.255. + /// \sa applyMask(const Address &) + void turnMaskedBitsOn(const Address &mask); + /** Return the ASCII equivalent of the address * Semantically equivalent to the IPv4 inet_ntoa() * eg. 127.0.0.1 (IPv4) or ::1 (IPv6) diff --git a/test-suite/Makefile.am b/test-suite/Makefile.am index b46d3851b4..3f4bc4cd2a 100644 --- a/test-suite/Makefile.am +++ b/test-suite/Makefile.am @@ -151,4 +151,9 @@ squid-conf-tests: $(srcdir)/test-squid-conf.sh $(top_builddir)/src/squid.conf.de done; \ if test "$$failed" -eq 0; then cp $(TRUE) $@ ; else exit 1; fi -CLEANFILES += squid-conf-tests squid-stderr.log +CLEANFILES += \ + squid-conf-tests \ + squid-expected-messages \ + squid-stderr.log \ + squid-stderr.log.next \ + squid-stderr.log.unmatched diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf new file mode 100644 index 0000000000..11ead73a69 --- /dev/null +++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf @@ -0,0 +1,19 @@ +## 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 test11 dstdomain .example.com example.com +acl test12 dstdomain example.com .example.com + +acl test21 dstdomain .example.com a.example.com +acl test22 dstdomain b.example.com .example.com + +acl test31 dstdomain .example.com .example.com c.example.com + +acl test41 dstdomain .d.example.com .example.com +acl test42 dstdomain .example.com .e.example.com + +acl test51 dstdomain example.com example.net . diff --git a/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions new file mode 100644 index 0000000000..868234ee65 --- /dev/null +++ b/test-suite/squidconf/bad-acl-dstdomain-dupe.conf.instructions @@ -0,0 +1,25 @@ +expect-messages < $messageRegexFilename + then + echo "$where: ERROR: Cannot create a temporary file named $messageRegexFilename" + exit 1 + fi + + local foundTerminator=0; + while read hereDocLine + do + lineNo=$(($lineNo+1)) + where="$instructionsFile:$lineNo"; + + if test "<<$hereDocLine" = "$heredocTerminator" + then + foundTerminator=1 + break; + fi + + # skip empty lines; they cannot be used as regexes and they improve + # here-document formatting + if test -z "$hereDocLine" + then + continue; + fi + + if ! printf '%s\n' "$hereDocLine" >> $messageRegexFilename + then + echo "$where: ERROR: Cannot write to a temporary file named $messageRegexFilename" + exit 1 + fi + done + + if test $foundTerminator != 1 + then + echo "$where: ERROR: Input ended before here-doc terminator ($heredocTerminator)" + exit 1 + fi +} + +# Checks that each of the extended regexes (in the given file) matches line(s) +# in the given Squid log file. A log line can match at most once. +matchEachRegex() +{ + local regexFilename="$1" + local errLog="$2" + + local errorLogRemaining="$errorLog.unmatched"; + local errorLogNext="$errorLog.next"; + + if ! cp $errorLog $errorLogRemaining + then + echo "ERROR: Cannot create a temporary file named $errorLogRemaining" + exit 1 + fi + + local result=0 + while read regex + do + if grep -q -E "$regex" $errorLogRemaining + then + # No good way to distinguish a possible lack of "grep -v" matches + # from grep errors because both result in non-zero grep exit code. + # For now, assume that error log always has some "extra" lines, + # guaranteeing at least one "grep -v non-empty-regex" match. + if ! grep -v -E "$regex" $errorLogRemaining > $errorLogNext || ! mv $errorLogNext $errorLogRemaining + then + echo "ERROR: Temporary file manipulation failure" + exit 1 + fi + else + echo "ERROR: Squid did not emit an expected message to stderr" + echo " expected message regex: $regex" + result=1 + fi + done < $regexFilename + + if test $result != 0 + then + echo "Unmatched Squid stderr lines (see $errorLogRemaining):" + cat $errorLogRemaining + fi + + return $result +} + instructionsFile="$configFile.instructions" if test -e $instructionsFile then @@ -75,6 +194,12 @@ then continue; fi + if test "$instructionName" = "expect-messages" + then + expectMessages "$p1" "$p2" "$here" + continue; + fi + if test "$instructionName" = "skip-unless-autoconf-defines" then # Skip test unless the given macro is #defined in autoconf.h @@ -136,6 +261,12 @@ then updateOutcome 1 fi +if test -n "$messageRegexFilename" && ! matchEachRegex $messageRegexFilename $errorLog +then + # matchEachRegex reports errors + updateOutcome 1 +fi + if test $expectFailure = no then if test "$result" -ne 0