From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 5 Aug 2024 19:30:08 +0000 (+0000)
Subject: Extend in-use ACLs lifetime across reconfiguration (#1826)
X-Git-Tag: SQUID_7_0_1~85
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0d6454140f07311bac841acdca6eab15548bf747;p=thirdparty%2Fsquid.git

Extend in-use ACLs lifetime across reconfiguration (#1826)

When Squid is reconfigured, all previously created Acl::Node objects
become unusable[^1], resulting in ACLChecklist::resumeNonBlockingCheck()
ACCESS_DUNNO outcomes. ACCESS_DUNNO leads to Squid applying the affected
directive "default" action rather than the configured one (e.g., an
http_access denies a transaction that should be allowed or miss_access
allows a transaction that should be denied). These unwanted and
surprising effects make hot reconfiguration unreliable. They are also
difficult to discover and triage.

An Acl::Node object should last as long as the object/scope that saved
that ACL for reuse. This change applies this (re)configuration principle
to ACLChecklist::accessList by converting that raw Acl::Tree pointer to
use reference counting. That change requires reference counting of
individual Acl::Node objects and related types. Several TODOs and XXXs
asked for reference counted ACLs, but more than 50 explicit raw ACL
pointers in Squid code make that transition difficult:

* Config.accessList.foo and similar items used raw Acl::Tree pointers to
  objects destroyed during reconfiguration by aclDestroyAcls(). They now
  point to refcounted Acl::Trees that ACLChecklist and other
  configuration-using code can safely store across reconfigurations. The
  outer Config.accessList.foo pointer is still raw because Squid
  invasive RefCount smart pointers cannot be stored directly in various
  configuration objects without significant out-of-scope refactoring.

* Acl::Node::next items formed an invasive list of raw ACL pointers
  (rooted in Config.aclList). The list was used to FindByName() and also
  to prepareForUse() explicitly named ACLs. Reconfiguration destroyed
  listed nodes via aclDestroyAcls(). Acl::Node::next is now gone.

* RegisteredAcls std::set stored raw ACL pointers to destroy all ACL
  objects (named and internally-generated) during reconfiguration, after
  all the free_foo() calls, in aclDestroyAcls(). It is now gone.

* InnerNode::nodes std::vector stored raw pointers to
  internally-generated Acl::Node objects. They are now refcounted.

The remaining raw Acl::Node pointers are not actually _stored_ across
async calls. They are used to pass ACLs around. Some of them should be
replaced with references (where it is known that the ACL must exist and
is not expected to be stored), but that is a different polishing issue.
For now, we just make sure that no raw Acl::Node pointers are stored
across async calls (because any stored raw ACL pointer may be
invalidated by reconfiguration).

Extending lifetime of in-use ACLs is necessary but not sufficient to
make Squid reconfiguration reliable. We need to preserve more in-use
resources...

[^1]: Prior to these changes, reconfiguration made Acl::Node objects
unusable because their C++ destructors were called. Cbdata locks may
have kept underlying object memory, but any linked resources freed by
destructors were gone, and the destructed objects could not be accessed.
---

diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc
index 0e405eb85a..d9e7a2456e 100644
--- a/src/ConfigParser.cc
+++ b/src/ConfigParser.cc
@@ -594,13 +594,13 @@ ConfigParser::skipOptional(const char *keyword)
     return false; // no more tokens (i.e. we are at the end of the line)
 }
 
-Acl::Tree *
+ACLList *
 ConfigParser::optionalAclList()
 {
     if (!skipOptional("if"))
         return nullptr; // OK: the directive has no ACLs
 
-    Acl::Tree *acls = nullptr;
+    ACLList *acls = nullptr;
     const auto aclCount = aclParseAclList(*this, &acls, cfg_directive);
     assert(acls);
     if (aclCount <= 0)
diff --git a/src/ConfigParser.h b/src/ConfigParser.h
index 4b588bcaee..1ef9577b4a 100644
--- a/src/ConfigParser.h
+++ b/src/ConfigParser.h
@@ -72,7 +72,7 @@ public:
     bool skipOptional(const char *keyword);
 
     /// parses an [if [!]<acl>...] construct
-    Acl::Tree *optionalAclList();
+    ACLList *optionalAclList();
 
     /// extracts and returns a regex (including any optional flags)
     std::unique_ptr<RegexPattern> regex(const char *expectedRegexDescription);
diff --git a/src/Notes.cc b/src/Notes.cc
index 1f60d37f2e..3f77761f5c 100644
--- a/src/Notes.cc
+++ b/src/Notes.cc
@@ -110,7 +110,7 @@ Note::printAsNoteDirective(StoreEntry * const entry, const char * const directiv
         os << directiveName << ' ' << key() << ' ' << ConfigParser::QuoteString(SBufToString(v->value()));
         if (v->aclList) {
             // TODO: Use Acl::dump() after fixing the XXX in dump_acl_list().
-            for (const auto &item: v->aclList->treeDump("", &Acl::AllowOrDeny)) {
+            for (const auto &item: ToTree(v->aclList).treeDump("", &Acl::AllowOrDeny)) {
                 if (item.isEmpty()) // treeDump("") adds this prefix
                     continue;
                 if (item.cmp("\n") == 0) // treeDump() adds this suffix
diff --git a/src/SquidConfig.h b/src/SquidConfig.h
index 8ac9a47b94..8f5063d780 100644
--- a/src/SquidConfig.h
+++ b/src/SquidConfig.h
@@ -353,7 +353,7 @@ public:
 
     std::chrono::nanoseconds paranoid_hit_validation;
 
-    class Acl::Node *aclList;
+    Acl::NamedAcls *namedAcls; ///< acl aclname acltype ...
 
     struct {
         acl_access *http;
diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc
index b08ab442e5..2d07d3fc9a 100644
--- a/src/acl/Acl.cc
+++ b/src/acl/Acl.cc
@@ -20,15 +20,24 @@
 #include "debug/Stream.h"
 #include "fatal.h"
 #include "globals.h"
+#include "mem/PoolingAllocator.h"
+#include "sbuf/Algorithms.h"
 #include "sbuf/List.h"
 #include "sbuf/Stream.h"
 #include "SquidConfig.h"
 
 #include <algorithm>
 #include <map>
+#include <unordered_map>
 
 namespace Acl {
 
+/// parsed "acl aclname ..." directives indexed by aclname
+class NamedAcls: public std::unordered_map<SBuf, Acl::Node::Pointer,
+    CaseInsensitiveSBufHash, CaseInsensitiveSBufEqual,
+    PoolingAllocator< std::pair<const SBuf, Acl::Node::Pointer> > > {
+};
+
 /// Acl::Node type name comparison functor
 class TypeNameCmp {
 public:
@@ -149,40 +158,30 @@ void Acl::Node::operator delete(void *)
     fatal ("unusable Acl::Node::delete");
 }
 
-/// implements both Acl::Node::FindByName() variations
-template <typename SBufOrCString>
-static Acl::Node *
-FindByNameT(const SBufOrCString name)
+Acl::Node *
+Acl::Node::FindByName(const SBuf &name)
 {
-    for (auto a = Config.aclList; a; a = a->next) {
-        if (a->name.caseCmp(name) == 0) {
-            debugs(28, 8, "found " << a->name);
-            return a;
-        }
+    if (!Config.namedAcls) {
+        debugs(28, 8, "no named ACLs to find " << name);
+        return nullptr;
     }
 
-    debugs(28, 8, "cannot find " << name);
-    return nullptr;
-}
+    const auto result = Config.namedAcls->find(name);
+    if (result == Config.namedAcls->end()) {
+        debugs(28, 8, "no ACL named " << name);
+        return nullptr;
+    }
 
-Acl::Node *
-Acl::Node::FindByName(const SBuf &name)
-{
-    return FindByNameT(name);
+    debugs(28, 8, result->second << " is named " << name);
+    assert(result->second);
+    return result->second.getRaw();
 }
 
-Acl::Node *
-Acl::Node::FindByName(const char *name)
+Acl::Node::Node()
 {
-    return FindByNameT(name);
+    debugs(28, 8, "constructing, this=" << this);
 }
 
-Acl::Node::Node() :
-    cfgline(nullptr),
-    next(nullptr),
-    registered(false)
-{}
-
 bool
 Acl::Node::valid() const
 {
@@ -230,7 +229,7 @@ Acl::Node::context(const SBuf &aName, const char *aCfgLine)
 }
 
 void
-Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
+Acl::Node::ParseNamedAcl(ConfigParser &parser, NamedAcls *&namedAcls)
 {
     /* we're already using strtok() to grok the line */
     char *t = nullptr;
@@ -243,15 +242,18 @@ Acl::Node::ParseAclLine(ConfigParser &parser, Node ** head)
         return;
     }
 
+    if (!namedAcls)
+        namedAcls = new NamedAcls();
+
     SBuf aclname(t);
     CallParser(ParsingContext::Pointer::Make(aclname), [&] {
-        ParseNamed(parser, head, aclname);
+        ParseNamed(parser, *namedAcls, aclname);
     });
 }
 
 /// parses acl directive parts that follow aclname
 void
-Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &aclname)
+Acl::Node::ParseNamed(ConfigParser &parser, NamedAcls &namedAcls, const SBuf &aclname)
 {
     /* snarf the ACL type */
     const char *theType;
@@ -293,9 +295,9 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &acln
         theType = "client_connection_mark";
     }
 
-    Node *A = nullptr;
+    auto A = FindByName(aclname);
     int new_acl = 0;
-    if ((A = FindByName(aclname)) == nullptr) {
+    if (!A) {
         debugs(28, 3, "aclParseAclLine: Creating ACL '" << aclname << "'");
         A = Acl::Make(theType);
         A->context(aclname, config_input_line);
@@ -328,13 +330,27 @@ Acl::Node::ParseNamed(ConfigParser &parser, Node ** const head, const SBuf &acln
                A->cfgline);
     }
 
-    // add to the global list for searching explicit ACLs by name
-    assert(head && *head == Config.aclList);
-    A->next = *head;
-    *head = A;
+    const auto insertion = namedAcls.emplace(A->name, A);
+    Assure(insertion.second); // FindByName() above checked that A is a new ACL
+}
 
-    // register for centralized cleanup
-    aclRegister(A);
+void
+Acl::DumpNamedAcls(std::ostream &os, const char * const directiveName, NamedAcls * const namedAcls)
+{
+    if (namedAcls) {
+        for (const auto &nameAndAcl: *namedAcls) {
+            debugs(3, 3, directiveName << ' ' << nameAndAcl.first);
+            nameAndAcl.second->dumpWhole(directiveName, os);
+        }
+    }
+}
+
+void
+Acl::FreeNamedAcls(NamedAcls ** const namedAcls)
+{
+    assert(namedAcls);
+    delete *namedAcls;
+    *namedAcls = nullptr;
 }
 
 bool
@@ -450,19 +466,17 @@ Acl::Node::requiresRequest() const
 
 Acl::Node::~Node()
 {
-    debugs(28, 3, "freeing ACL " << name);
+    debugs(28, 8, "destructing " <<  name << ", this=" << this);
     safe_free(cfgline);
 }
 
 void
 Acl::Node::Initialize()
 {
-    auto *a = Config.aclList;
-    debugs(53, 3, "Acl::Node::Initialize");
-
-    while (a) {
-        a->prepareForUse();
-        a = a->next;
+    debugs(28, 3, (Config.namedAcls ? Config.namedAcls->size() : 0));
+    if (Config.namedAcls) {
+        for (const auto &nameAndAcl: *Config.namedAcls)
+            nameAndAcl.second->prepareForUse();
     }
 }
 
diff --git a/src/acl/Acl.h b/src/acl/Acl.h
index 130682b2c4..5b46121f7d 100644
--- a/src/acl/Acl.h
+++ b/src/acl/Acl.h
@@ -125,6 +125,12 @@ operator <<(std::ostream &o, const Answer &a)
     return o;
 }
 
+/// report the given list of "acl" directives (using squid.conf syntax)
+void DumpNamedAcls(std::ostream &, const char *directiveName, NamedAcls *);
+
+/// delete the given list of "acl" directives
+void FreeNamedAcls(NamedAcls **);
+
 } // namespace Acl
 
 /// \ingroup ACLAPI
diff --git a/src/acl/AllOf.cc b/src/acl/AllOf.cc
index 731f56781b..56002e7ed3 100644
--- a/src/acl/AllOf.cc
+++ b/src/acl/AllOf.cc
@@ -10,7 +10,6 @@
 #include "acl/AllOf.h"
 #include "acl/BoolOps.h"
 #include "acl/Checklist.h"
-#include "acl/Gadgets.h"
 #include "cache_cf.h"
 #include "MemBuf.h"
 #include "sbuf/SBuf.h"
@@ -37,7 +36,7 @@ Acl::AllOf::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const
     if (empty())
         return 1; // not 0 because in math empty product equals identity
 
-    if (checklist->matchChild(this, start, *start))
+    if (checklist->matchChild(this, start))
         return 1; // match
 
     return checklist->keepMatching() ? 0 : -1;
@@ -48,7 +47,7 @@ void
 Acl::AllOf::parse()
 {
     Acl::InnerNode *whole = nullptr;
-    Acl::Node *oldNode = empty() ? nullptr : nodes.front();
+    const auto oldNode = empty() ? nullptr : nodes.front().getRaw();
 
     // optimization: this logic reduces subtree hight (number of tree levels)
     if (Acl::OrNode *oldWhole = dynamic_cast<Acl::OrNode*>(oldNode)) {
@@ -61,7 +60,6 @@ Acl::AllOf::parse()
         newWhole->context(ToSBuf('(', name, " lines)"), oldNode->cfgline);
         newWhole->add(oldNode); // old (i.e. first) line
         nodes.front() = whole = newWhole;
-        aclRegister(newWhole);
     } else {
         // this is the first line for this acl; just use it as is
         whole = this;
diff --git a/src/acl/BoolOps.cc b/src/acl/BoolOps.cc
index e418c567b2..06508d32ec 100644
--- a/src/acl/BoolOps.cc
+++ b/src/acl/BoolOps.cc
@@ -36,7 +36,7 @@ Acl::NotNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) cons
 {
     assert(start == nodes.begin()); // we only have one node
 
-    if (checklist->matchChild(this, start, *start))
+    if (checklist->matchChild(this, start))
         return 0; // converting match into mismatch
 
     if (!checklist->keepMatching())
@@ -72,7 +72,7 @@ Acl::AndNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) cons
 {
     // find the first node that does not match
     for (Nodes::const_iterator i = start; i != nodes.end(); ++i) {
-        if (!checklist->matchChild(this, i, *i))
+        if (!checklist->matchChild(this, i))
             return checklist->keepMatching() ? 0 : -1;
     }
 
@@ -110,7 +110,7 @@ Acl::OrNode::doMatch(ACLChecklist *checklist, Nodes::const_iterator start) const
     for (Nodes::const_iterator i = start; i != nodes.end(); ++i) {
         if (bannedAction(checklist, i))
             continue;
-        if (checklist->matchChild(this, i, *i)) {
+        if (checklist->matchChild(this, i)) {
             lastMatch_ = i;
             return 1;
         }
diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc
index 4dc5ad7e39..950d6fe060 100644
--- a/src/acl/Checklist.cc
+++ b/src/acl/Checklist.cc
@@ -27,17 +27,6 @@ ACLChecklist::prepNonBlocking()
         return false;
     }
 
-    /** \par
-     * If the accessList is no longer valid (i.e. its been
-     * freed because of a reconfigure), then bail with ACCESS_DUNNO.
-     */
-
-    if (!cbdataReferenceValid(accessList)) {
-        cbdataReferenceDone(accessList);
-        checkCallback("accessList is invalid");
-        return false;
-    }
-
     return true;
 }
 
@@ -49,7 +38,6 @@ ACLChecklist::completeNonBlocking()
     if (!finished())
         calcImplicitAnswer();
 
-    cbdataReferenceDone(accessList);
     checkCallback(nullptr);
 }
 
@@ -79,8 +67,9 @@ ACLChecklist::preCheck(const char *what)
 }
 
 bool
-ACLChecklist::matchChild(const Acl::InnerNode *current, Acl::Nodes::const_iterator pos, const Acl::Node *child)
+ACLChecklist::matchChild(const Acl::InnerNode * const current, const Acl::Nodes::const_iterator pos)
 {
+    const auto &child = *pos;
     assert(current && child);
 
     // Remember the current tree location to prevent "async loop" cases where
@@ -191,10 +180,21 @@ ACLChecklist::ACLChecklist() :
 ACLChecklist::~ACLChecklist()
 {
     assert (!asyncInProgress());
+    debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this);
+}
 
-    changeAcl(nullptr);
+void
+ACLChecklist::changeAcl(const acl_access * const replacement)
+{
+    accessList = replacement ? *replacement : nullptr;
+}
 
-    debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this);
+Acl::TreePointer
+ACLChecklist::swapAcl(const acl_access * const replacement)
+{
+    const auto old = accessList;
+    changeAcl(replacement);
+    return old;
 }
 
 /**
@@ -269,24 +269,24 @@ ACLChecklist::matchAndFinish()
         markFinished(accessList->winningAction(), "match");
 }
 
-Acl::Answer const &
-ACLChecklist::fastCheck(const Acl::Tree * list)
+const Acl::Answer &
+ACLChecklist::fastCheck(const ACLList * const list)
 {
     preCheck("fast ACLs");
     asyncCaller_ = false;
 
     // Concurrent checks are not supported, but sequential checks are, and they
     // may use a mixture of fastCheck(void) and fastCheck(list) calls.
-    const Acl::Tree * const savedList = changeAcl(list);
+    const auto savedList = swapAcl(list);
 
     // assume DENY/ALLOW on mis/matches due to action-free accessList
     // matchAndFinish() takes care of the ALLOW case
-    if (accessList && cbdataReferenceValid(accessList))
+    if (accessList)
         matchAndFinish(); // calls markFinished() on success
     if (!finished())
         markFinished(ACCESS_DENIED, "ACLs failed to match");
 
-    changeAcl(savedList);
+    changeAcl(&savedList);
     occupied_ = false;
     return currentAnswer();
 }
@@ -301,13 +301,11 @@ ACLChecklist::fastCheck()
     asyncCaller_ = false;
 
     debugs(28, 5, "aclCheckFast: list: " << accessList);
-    const Acl::Tree *acl = cbdataReference(accessList);
-    if (acl != nullptr && cbdataReferenceValid(acl)) {
+    if (accessList) {
         matchAndFinish(); // calls markFinished() on success
 
         // if finished (on a match or in exceptional cases), stop
         if (finished()) {
-            cbdataReferenceDone(acl);
             occupied_ = false;
             return currentAnswer();
         }
@@ -317,7 +315,6 @@ ACLChecklist::fastCheck()
 
     // There were no rules to match or no rules matched
     calcImplicitAnswer();
-    cbdataReferenceDone(acl);
     occupied_ = false;
 
     return currentAnswer();
@@ -328,7 +325,7 @@ ACLChecklist::fastCheck()
 void
 ACLChecklist::calcImplicitAnswer()
 {
-    const auto lastAction = (accessList && cbdataReferenceValid(accessList)) ?
+    const auto lastAction = accessList ?
                             accessList->lastAction() : Acl::Answer(ACCESS_DUNNO);
     auto implicitRuleAnswer = Acl::Answer(ACCESS_DUNNO);
     if (lastAction == ACCESS_DENIED) // reverse last seen "deny"
diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h
index 1089991694..664751a1fa 100644
--- a/src/acl/Checklist.h
+++ b/src/acl/Checklist.h
@@ -104,15 +104,15 @@ public:
      *
      * If there are no ACLs to check at all, the result becomes ACCESS_ALLOWED.
      */
-    Acl::Answer const & fastCheck(const Acl::Tree *list);
+    const Acl::Answer &fastCheck(const ACLList *);
 
     /// If slow lookups are allowed, switches into "async in progress" state.
     /// Otherwise, returns false; the caller is expected to handle the failure.
     bool goAsync(AsyncStarter, const Acl::Node &);
 
-    /// Matches (or resumes matching of) a child node while maintaning
+    /// Matches (or resumes matching of) a child node at pos while maintaining
     /// resumption breadcrumbs if a [grand]child node goes async.
-    bool matchChild(const Acl::InnerNode *parent, Acl::Nodes::const_iterator pos, const Acl::Node *child);
+    bool matchChild(const Acl::InnerNode *parent, Acl::Nodes::const_iterator pos);
 
     /// Whether we should continue to match tree nodes or stop/pause.
     bool keepMatching() const { return !finished() && !asyncInProgress(); }
@@ -144,15 +144,7 @@ public:
     virtual void verifyAle() const = 0;
 
     /// change the current ACL list
-    /// \return a pointer to the old list value (may be nullptr)
-    const Acl::Tree *changeAcl(const Acl::Tree *t) {
-        const Acl::Tree *old = accessList;
-        if (t != accessList) {
-            cbdataReferenceDone(accessList);
-            accessList = cbdataReference(t);
-        }
-        return old;
-    }
+    void changeAcl(const acl_access *);
 
     /// remember the name of the last ACL being evaluated
     void setLastCheckedName(const SBuf &name) { lastCheckedName_ = name; }
@@ -164,7 +156,12 @@ private:
 
     void matchAndFinish();
 
-    const Acl::Tree *accessList;
+    /// \copydoc changeAcl()
+    /// \returns old accessList pointer (that may be nil)
+    Acl::TreePointer swapAcl(const acl_access *);
+
+    Acl::TreePointer accessList;
+
 public:
 
     ACLCB *callback;
@@ -184,7 +181,7 @@ private: /* internal methods */
         bool operator ==(const Breadcrumb &b) const { return parent == b.parent && (!parent || position == b.position); }
         bool operator !=(const Breadcrumb &b) const { return !this->operator ==(b); }
         void clear() { parent = nullptr; }
-        const Acl::InnerNode *parent; ///< intermediate node in the ACL tree
+        RefCount<const Acl::InnerNode> parent; ///< intermediate node in the ACL tree
         Acl::Nodes::const_iterator position; ///< child position inside parent
     };
 
diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc
index fb0492cf41..d12040dc33 100644
--- a/src/acl/Gadgets.cc
+++ b/src/acl/Gadgets.cc
@@ -28,13 +28,8 @@
 #include "SquidConfig.h"
 #include "src/sbuf/Stream.h"
 
-#include <set>
 #include <algorithm>
 
-using AclSet = std::set<Acl::Node *>;
-/// Accumulates all ACLs to facilitate their clean deletion despite reuse.
-static AclSet *RegisteredAcls; // TODO: Remove when ACLs are refcounted
-
 err_type
 FindDenyInfoPage(const Acl::Answer &answer, const bool redirect_allowed)
 {
@@ -124,8 +119,17 @@ aclParseDenyInfoLine(AclDenyInfoList ** head)
     *T = A;
 }
 
+const Acl::Tree &
+Acl::ToTree(const TreePointer * const config)
+{
+    Assure(config);
+    const auto &treePtr = *config;
+    Assure(treePtr);
+    return *treePtr;
+}
+
 void
-aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep)
+aclParseAccessLine(const char *directive, ConfigParser &, acl_access **config)
 {
     /* first expect either 'allow' or 'deny' */
     const char *t = ConfigParser::NextToken();
@@ -147,7 +151,7 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep)
         return;
     }
 
-    const int ruleId = ((treep && *treep) ? (*treep)->childrenCount() : 0) + 1;
+    const int ruleId = ((config && *config) ? ToTree(*config).childrenCount() : 0) + 1;
 
     Acl::AndNode *rule = new Acl::AndNode;
     rule->context(ToSBuf(directive, '#', ruleId), config_input_line);
@@ -161,6 +165,11 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep)
 
     /* Append to the end of this list */
 
+    assert(config);
+    if (!*config)
+        *config = new acl_access();
+    const auto treep = *config;
+
     assert(treep);
     if (!*treep) {
         *treep = new Acl::Tree;
@@ -168,13 +177,11 @@ aclParseAccessLine(const char *directive, ConfigParser &, acl_access **treep)
     }
 
     (*treep)->add(rule, action);
-
-    /* We lock _acl_access structures in ACLChecklist::matchNonBlocking() */
 }
 
 // aclParseAclList does not expect or set actions (cf. aclParseAccessLine)
 size_t
-aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label)
+aclParseAclList(ConfigParser &, ACLList **config, const char *label)
 {
     // accommodate callers unable to convert their ACL list context to string
     if (!label)
@@ -184,64 +191,25 @@ aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label)
     rule->context(ToSBuf('(', cfg_directive, ' ', label, " line)"), config_input_line);
     const auto aclCount = rule->lineParse();
 
-    // We want a cbdata-protected Tree (despite giving it only one child node).
+    // XXX: We have created only one node, and our callers do not support
+    // actions, but we now have to create an action-supporting Acl::Tree because
+    // Checklist needs actions support. TODO: Add actions methods to Acl::Node,
+    // so that Checklist can be satisfied with Acl::AndNode created above.
     Acl::Tree *tree = new Acl::Tree;
     tree->add(rule);
     tree->context(ToSBuf(cfg_directive, ' ', label), config_input_line);
 
-    assert(treep);
-    assert(!*treep);
-    *treep = tree;
+    assert(config);
+    assert(!*config);
+    *config = new acl_access(tree);
 
     return aclCount;
 }
 
-void
-aclRegister(Acl::Node *acl)
-{
-    if (!acl->registered) {
-        if (!RegisteredAcls)
-            RegisteredAcls = new AclSet;
-        RegisteredAcls->insert(acl);
-        acl->registered = true;
-    }
-}
-
-/// remove registered acl from the centralized deletion set
-static
-void
-aclDeregister(Acl::Node *acl)
-{
-    if (acl->registered) {
-        if (RegisteredAcls)
-            RegisteredAcls->erase(acl);
-        acl->registered = false;
-    }
-}
-
 /*********************/
 /* Destroy functions */
 /*********************/
 
-/// called to delete ALL Acls.
-void
-aclDestroyAcls(Acl::Node ** head)
-{
-    *head = nullptr; // Config.aclList
-    if (AclSet *acls = RegisteredAcls) {
-        debugs(28, 8, "deleting all " << acls->size() << " ACLs");
-        while (!acls->empty()) {
-            auto *acl = *acls->begin();
-            // We use centralized deletion (this function) so ~Acl::Node should not
-            // delete other ACLs, but we still deregister first to prevent any
-            // accesses to the being-deleted Acl::Node via RegisteredAcls.
-            assert(acl->registered); // make sure we are making progress
-            aclDeregister(acl);
-            delete acl;
-        }
-    }
-}
-
 void
 aclDestroyAclList(ACLList **list)
 {
@@ -256,7 +224,7 @@ aclDestroyAccessList(acl_access ** list)
 {
     assert(list);
     if (*list)
-        debugs(28, 3, "destroying: " << *list << ' ' << (*list)->name);
+        debugs(28, 3, "destroying: " << *list << ' ' << ToTree(*list).name);
     delete *list;
     *list = nullptr;
 }
diff --git a/src/acl/Gadgets.h b/src/acl/Gadgets.h
index 1e09be11b1..6df2fa7b16 100644
--- a/src/acl/Gadgets.h
+++ b/src/acl/Gadgets.h
@@ -21,26 +21,23 @@ class dlink_list;
 class StoreEntry;
 class wordlist;
 
-/// Register an Acl::Node object for future deletion. Repeated registrations are OK.
-/// \ingroup ACLAPI
-void aclRegister(Acl::Node *acl);
 /// \ingroup ACLAPI
 void aclDestroyAccessList(acl_access **list);
 /// \ingroup ACLAPI
-void aclDestroyAcls(Acl::Node **);
-/// \ingroup ACLAPI
 void aclDestroyAclList(ACLList **);
+
 /// Parses a single line of a "action followed by acls" directive (e.g., http_access).
-/// \ingroup ACLAPI
-void aclParseAccessLine(const char *directive, ConfigParser &parser, Acl::Tree **);
+void aclParseAccessLine(const char *directive, ConfigParser &, acl_access **);
+
 /// Parses a single line of a "some context followed by acls" directive (e.g., note n v).
 /// The label parameter identifies the context (for debugging).
 /// \returns the number of parsed ACL names
-size_t aclParseAclList(ConfigParser &parser, Acl::Tree **, const char *label);
+size_t aclParseAclList(ConfigParser &, ACLList **, const char *label);
+
 /// Template to convert various context labels to strings. \ingroup ACLAPI
 template <class Any>
 inline size_t
-aclParseAclList(ConfigParser &parser, Acl::Tree **tree, const Any any)
+aclParseAclList(ConfigParser &parser, ACLList ** const tree, const Any any)
 {
     std::ostringstream buf;
     buf << any;
@@ -68,5 +65,14 @@ void dump_acl_access(StoreEntry * entry, const char *name, acl_access * head);
 /// \ingroup ACLAPI
 void dump_acl_list(StoreEntry * entry, ACLList * head);
 
+namespace Acl {
+/// convenient and safe access to a stored (and parsed/configured) Tree
+/// \returns **cfg or *cfg->getRaw()
+/// \prec cfg points to a non-nil TreePointer object; ACL parsing code is
+/// written so that ToTree() caller may just check that cfg itself is not nil
+/// (because parsing code never stores nil TreePointer objects).
+const Tree &ToTree(const TreePointer *cfg);
+}
+
 #endif /* SQUID_SRC_ACL_GADGETS_H */
 
diff --git a/src/acl/InnerNode.cc b/src/acl/InnerNode.cc
index 761b56814c..5b38472ea5 100644
--- a/src/acl/InnerNode.cc
+++ b/src/acl/InnerNode.cc
@@ -37,7 +37,6 @@ Acl::InnerNode::add(Acl::Node *node)
 {
     assert(node != nullptr);
     nodes.push_back(node);
-    aclRegister(node);
 }
 
 // kids use this method to handle [multiple] parse() calls correctly
@@ -57,7 +56,8 @@ Acl::InnerNode::lineParse()
             ++t;
 
         debugs(28, 3, "looking for ACL " << t);
-        auto *a = Acl::Node::FindByName(t);
+        // XXX: Performance regression: SBuf will allocate memory.
+        const auto a = Acl::Node::FindByName(SBuf(t));
 
         if (a == nullptr) {
             debugs(28, DBG_CRITICAL, "ERROR: ACL not found: " << t);
diff --git a/src/acl/InnerNode.h b/src/acl/InnerNode.h
index 63f36b7fa1..0d227f519e 100644
--- a/src/acl/InnerNode.h
+++ b/src/acl/InnerNode.h
@@ -15,14 +15,13 @@
 namespace Acl
 {
 
-using Nodes = std::vector<Acl::Node*>; ///< a collection of nodes
+/// operands of a boolean ACL expression, in configuration/evaluation order
+using Nodes = std::vector<Node::Pointer>;
 
 /// An intermediate Acl::Node tree node. Manages a collection of child tree nodes.
 class InnerNode: public Acl::Node
 {
 public:
-    // No ~InnerNode() to delete children. They are aclRegister()ed instead.
-
     /// Resumes matching (suspended by an async call) at the given position.
     bool resumeMatchingAt(ACLChecklist *checklist, Acl::Nodes::const_iterator pos) const;
 
@@ -49,8 +48,7 @@ protected:
     /* Acl::Node API */
     int match(ACLChecklist *checklist) override;
 
-    // XXX: use refcounting instead of raw pointers
-    Nodes nodes; ///< children nodes of this intermediate node
+    Nodes nodes; ///< children of this intermediate node
 };
 
 } // namespace Acl
diff --git a/src/acl/Node.h b/src/acl/Node.h
index cd264f5224..e1f8199766 100644
--- a/src/acl/Node.h
+++ b/src/acl/Node.h
@@ -22,21 +22,22 @@ namespace Acl {
 /// Can evaluate itself in FilledChecklist context.
 /// Does not change during evaluation.
 /// \ingroup ACLAPI
-class Node
+class Node: public RefCountable
 {
 
 public:
+    using Pointer = RefCount<Node>;
+
     void *operator new(size_t);
     void operator delete(void *);
 
-    static void ParseAclLine(ConfigParser &parser, Acl::Node **head);
+    /// parses acl directive parts that follow directive name (i.e. "acl")
+    static void ParseNamedAcl(ConfigParser &, NamedAcls *&);
+
     static void Initialize();
 
     /// A configured ACL with a given name or nil.
     static Acl::Node *FindByName(const SBuf &);
-    /// \copydoc FindByName()
-    /// \deprecated Use to avoid performance regressions; remove with the last caller.
-    static Acl::Node *FindByName(const char *name);
 
     Node();
     Node(Node &&) = delete;  // no copying of any kind
@@ -79,9 +80,7 @@ public:
     /// See also: context() and FindByName().
     SBuf name;
 
-    char *cfgline;
-    Acl::Node *next;  // XXX: remove or at least use refcounting
-    bool registered;  ///< added to the global list of ACLs via aclRegister()
+    char *cfgline = nullptr;
 
 private:
     /// Matches the actual data in checklist against this Acl::Node.
@@ -102,7 +101,7 @@ private:
     /// \see Acl::Node::options()
     virtual const Acl::Options &lineOptions() { return Acl::NoOptions(); }
 
-    static void ParseNamed(ConfigParser &, Node **head, const SBuf &name);
+    static void ParseNamed(ConfigParser &, NamedAcls &, const SBuf &name);
 };
 
 } // namespace Acl
diff --git a/src/acl/Tree.cc b/src/acl/Tree.cc
index 1ecf291e46..faa3d05f93 100644
--- a/src/acl/Tree.cc
+++ b/src/acl/Tree.cc
@@ -11,8 +11,6 @@
 #include "acl/Tree.h"
 #include "wordlist.h"
 
-CBDATA_NAMESPACED_CLASS_INIT(Acl, Tree);
-
 Acl::Answer
 Acl::Tree::winningAction() const
 {
diff --git a/src/acl/Tree.h b/src/acl/Tree.h
index 5e3a3381fe..63999762df 100644
--- a/src/acl/Tree.h
+++ b/src/acl/Tree.h
@@ -17,13 +17,11 @@
 namespace Acl
 {
 
-/// An ORed set of rules at the top of the ACL expression tree, providing two
-/// unique properties: cbdata protection and optional rule actions.
+/// An ORed set of rules at the top of the ACL expression tree with support for
+/// optional rule actions.
 class Tree: public OrNode
 {
-    // XXX: We should use refcounting instead, but it requires making ACLs
-    // refcounted as well. Otherwise, async lookups will reach deleted ACLs.
-    CBDATA_CLASS(Tree);
+    MEMPROXY_CLASS(Tree);
 
 public:
     /// dumps <name, action, rule, new line> tuples
diff --git a/src/acl/forward.h b/src/acl/forward.h
index d11747ff72..e47095dddf 100644
--- a/src/acl/forward.h
+++ b/src/acl/forward.h
@@ -13,7 +13,6 @@
 
 class ACLChecklist;
 class ACLFilledChecklist;
-class ACLList;
 
 class AclDenyInfoList;
 class AclSizeLimit;
@@ -27,6 +26,7 @@ class AndNode;
 class Answer;
 class ChecklistFiller;
 class InnerNode;
+class NamedAcls;
 class NotNode;
 class OrNode;
 class Tree;
@@ -34,14 +34,17 @@ class Tree;
 /// prepares to parse ACLs configuration
 void Init(void);
 
+/// reconfiguration-safe storage of ACL rules
+using TreePointer = RefCount<Acl::Tree>;
+
 } // namespace Acl
 
 typedef void ACLCB(Acl::Answer, void *);
 
 // TODO: Consider renaming all users and removing. Cons: hides the difference
 // between ACLList tree without actions and acl_access Tree with actions.
-#define acl_access Acl::Tree
-#define ACLList Acl::Tree
+using acl_access = Acl::TreePointer;
+using ACLList = Acl::TreePointer;
 
 class ExternalACLEntry;
 typedef RefCount<ExternalACLEntry> ExternalACLEntryPointer;
diff --git a/src/cache_cf.cc b/src/cache_cf.cc
index 10e15d270e..b66ba93cd9 100644
--- a/src/cache_cf.cc
+++ b/src/cache_cf.cc
@@ -1486,26 +1486,23 @@ free_SBufList(SBufList *list)
 }
 
 static void
-dump_acl(StoreEntry * entry, const char *name, Acl::Node * ae)
+dump_acl(StoreEntry *entry, const char *directiveName, Acl::NamedAcls *config)
 {
     PackableStream os(*entry);
-    while (ae != nullptr) {
-        debugs(3, 3, "dump_acl: " << name << " " << ae->name);
-        ae->dumpWhole(name, os);
-        ae = ae->next;
-    }
+    Acl::DumpNamedAcls(os, directiveName, config);
 }
 
 static void
-parse_acl(Acl::Node ** ae)
+parse_acl(Acl::NamedAcls **config)
 {
-    Acl::Node::ParseAclLine(LegacyParser, ae);
+    assert(config);
+    Acl::Node::ParseNamedAcl(LegacyParser, *config);
 }
 
 static void
-free_acl(Acl::Node ** ae)
+free_acl(Acl::NamedAcls **config)
 {
-    aclDestroyAcls(ae);
+    Acl::FreeNamedAcls(config);
 }
 
 void
@@ -1513,14 +1510,14 @@ dump_acl_list(StoreEntry * entry, ACLList * head)
 {
     // XXX: Should dump ACL names like "foo !bar" but dumps parsing context like
     // "(clientside_tos 0x11 line)".
-    dump_SBufList(entry, head->dump());
+    dump_SBufList(entry, ToTree(head).dump());
 }
 
 void
 dump_acl_access(StoreEntry * entry, const char *name, acl_access * head)
 {
     if (head)
-        dump_SBufList(entry, head->treeDump(name, &Acl::AllowOrDeny));
+        dump_SBufList(entry, ToTree(head).treeDump(name, &Acl::AllowOrDeny));
 }
 
 static void
@@ -2011,7 +2008,7 @@ static void
 dump_AuthSchemes(StoreEntry *entry, const char *name, acl_access *authSchemes)
 {
     if (authSchemes)
-        dump_SBufList(entry, authSchemes->treeDump(name, [](const Acl::Answer &action) {
+        dump_SBufList(entry, ToTree(authSchemes).treeDump(name, [](const Acl::Answer &action) {
         return Auth::TheConfig.schemeLists.at(action.kind).rawSchemes;
     }));
 }
@@ -2019,13 +2016,17 @@ dump_AuthSchemes(StoreEntry *entry, const char *name, acl_access *authSchemes)
 #endif /* USE_AUTH */
 
 static void
-ParseAclWithAction(acl_access **access, const Acl::Answer &action, const char *desc, Acl::Node *acl)
+ParseAclWithAction(acl_access **config, const Acl::Answer &action, const char *desc, Acl::Node *acl)
 {
-    assert(access);
-    if (!*access) {
-        *access = new Acl::Tree;
-        (*access)->context(ToSBuf('(', desc, " rules)"), config_input_line);
+    assert(config);
+    auto &access = *config;
+    if (!access) {
+        const auto tree = new Acl::Tree;
+        tree->context(ToSBuf('(', desc, " rules)"), config_input_line);
+        access = new acl_access(tree);
     }
+    assert(access);
+
     Acl::AndNode *rule = new Acl::AndNode;
     rule->context(ToSBuf('(', desc, " rule)"), config_input_line);
     if (acl)
@@ -4485,7 +4486,7 @@ static void parse_sslproxy_ssl_bump(acl_access **ssl_bump)
 static void dump_sslproxy_ssl_bump(StoreEntry *entry, const char *name, acl_access *ssl_bump)
 {
     if (ssl_bump)
-        dump_SBufList(entry, ssl_bump->treeDump(name, [](const Acl::Answer &action) {
+        dump_SBufList(entry, ToTree(ssl_bump).treeDump(name, [](const Acl::Answer &action) {
         return Ssl::BumpModeStr.at(action.kind);
     }));
 }
@@ -4729,7 +4730,7 @@ static void parse_ftp_epsv(acl_access **ftp_epsv)
 static void dump_ftp_epsv(StoreEntry *entry, const char *name, acl_access *ftp_epsv)
 {
     if (ftp_epsv)
-        dump_SBufList(entry, ftp_epsv->treeDump(name, Acl::AllowOrDeny));
+        dump_SBufList(entry, ToTree(ftp_epsv).treeDump(name, Acl::AllowOrDeny));
 }
 
 static void free_ftp_epsv(acl_access **ftp_epsv)
@@ -4900,7 +4901,7 @@ dump_on_unsupported_protocol(StoreEntry *entry, const char *name, acl_access *ac
         "respond"
     };
     if (access) {
-        SBufList lines = access->treeDump(name, [](const Acl::Answer &action) {
+        const auto lines = ToTree(access).treeDump(name, [](const Acl::Answer &action) {
             return onErrorTunnelMode.at(action.kind);
         });
         dump_SBufList(entry, lines);
@@ -4934,7 +4935,7 @@ dump_http_upgrade_request_protocols(StoreEntry *entry, const char *rawName, Http
         SBufList line;
         line.push_back(name);
         line.push_back(proto);
-        const auto acld = acls->treeDump("", &Acl::AllowOrDeny);
+        const auto acld = ToTree(acls).treeDump("", &Acl::AllowOrDeny);
         line.insert(line.end(), acld.begin(), acld.end());
         dump_SBufList(entry, line);
     });
diff --git a/src/cf.data.pre b/src/cf.data.pre
index 4ebdf93a51..bf8eb45ee4 100644
--- a/src/cf.data.pre
+++ b/src/cf.data.pre
@@ -1089,7 +1089,7 @@ DOC_END
 
 NAME: acl
 TYPE: acl
-LOC: Config.aclList
+LOC: Config.namedAcls
 IF USE_OPENSSL
 DEFAULT: ssl::certHasExpired ssl_error X509_V_ERR_CERT_HAS_EXPIRED
 DEFAULT: ssl::certNotYetValid ssl_error X509_V_ERR_CERT_NOT_YET_VALID
diff --git a/src/clients/Client.cc b/src/clients/Client.cc
index b5d8a8d2b7..551e82e936 100644
--- a/src/clients/Client.cc
+++ b/src/clients/Client.cc
@@ -551,7 +551,7 @@ Client::haveParsedReplyHeaders()
 bool
 Client::blockCaching()
 {
-    if (const Acl::Tree *acl = Config.accessList.storeMiss) {
+    if (const auto acl = Config.accessList.storeMiss) {
         // This relatively expensive check is not in StoreEntry::checkCachable:
         // That method lacks HttpRequest and may be called too many times.
         ACLFilledChecklist ch(acl, originalRequest().getRaw());
diff --git a/src/external_acl.cc b/src/external_acl.cc
index 87d60da86f..e32592bece 100644
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -592,6 +592,14 @@ copyResultsFromEntry(const HttpRequest::Pointer &req, const ExternalACLEntryPoin
 Acl::Answer
 ACLExternal::aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch) const
 {
+    // Despite its external_acl C++ type name, acl->def is not an ACL (i.e. not
+    // a reference-counted Acl::Node) and gets invalidated by reconfiguration.
+    // TODO: RefCount external_acl, so that we do not have to bail here.
+    if (!cbdataReferenceValid(acl->def)) {
+        debugs(82, 3, "cannot resume matching; external_acl gone");
+        return ACCESS_DUNNO;
+    }
+
     debugs(82, 9, "acl=\"" << acl->def->name << "\"");
     ExternalACLEntryPointer entry = ch->extacl_entry;
 
diff --git a/src/security/KeyLog.cc b/src/security/KeyLog.cc
index 219a229b69..fb97e05a30 100644
--- a/src/security/KeyLog.cc
+++ b/src/security/KeyLog.cc
@@ -62,7 +62,7 @@ Security::KeyLog::dump(std::ostream &os) const
     dumpOptions(os);
     if (aclList) {
         // TODO: Use Acl::dump() after fixing the XXX in dump_acl_list().
-        for (const auto &acl: aclList->treeDump("if", &Acl::AllowOrDeny))
+        for (const auto &acl: ToTree(aclList).treeDump("if", &Acl::AllowOrDeny))
             os << ' ' << acl;
     }
 }
diff --git a/src/tests/stub_acl.cc b/src/tests/stub_acl.cc
index 29c1027bc6..2d07fcd718 100644
--- a/src/tests/stub_acl.cc
+++ b/src/tests/stub_acl.cc
@@ -16,5 +16,5 @@
 #include "acl/forward.h"
 
 #include "acl/Gadgets.h"
-size_t aclParseAclList(ConfigParser &, Acl::Tree **, const char *) STUB_RETVAL(0)
+size_t aclParseAclList(ConfigParser &, ACLList **, const char *) STUB_RETVAL(0)
 
diff --git a/src/tests/testACLMaxUserIP.cc b/src/tests/testACLMaxUserIP.cc
index fea6a8492b..2c856c2196 100644
--- a/src/tests/testACLMaxUserIP.cc
+++ b/src/tests/testACLMaxUserIP.cc
@@ -15,6 +15,7 @@
 #include "auth/UserRequest.h"
 #include "compat/cppunit.h"
 #include "ConfigParser.h"
+#include "SquidConfig.h"
 #include "unitTestMain.h"
 
 #include <stdexcept>
@@ -74,9 +75,11 @@ TestACLMaxUserIP::testParseLine()
     char * line = xstrdup("test max_user_ip -s 1");
     /* seed the parser */
     ConfigParser::SetCfgLine(line);
-    Acl::Node *anACL = nullptr;
     ConfigParser LegacyParser;
-    Acl::Node::ParseAclLine(LegacyParser, &anACL);
+    Acl::Node::ParseNamedAcl(LegacyParser, Config.namedAcls);
+    CPPUNIT_ASSERT(Config.namedAcls);
+    const auto anACL = Acl::Node::FindByName(SBuf("test"));
+    CPPUNIT_ASSERT(anACL);
     ACLMaxUserIP *maxUserIpACL = dynamic_cast<ACLMaxUserIP *>(anACL);
     CPPUNIT_ASSERT(maxUserIpACL);
     if (maxUserIpACL) {
@@ -86,7 +89,7 @@ TestACLMaxUserIP::testParseLine()
         /* the acl must be vaid */
         CPPUNIT_ASSERT_EQUAL(true, maxUserIpACL->valid());
     }
-    delete anACL;
+    Acl::FreeNamedAcls(&Config.namedAcls);
     xfree(line);
 }