Centrally destroy all explicit and implicit ACLs to avoid destruction segfaults

during reconfiguration.

Group ACLs created later may use other ACLs created earlier and vice versa, a
group ACL created earlier may use other ACLs created later. The latter is
possible when an ACL (e.g., A2 below) is declared when the group already
exists:

  acl A1 src 127.0.0.1
  acl Group all-of A1
  acl A2 src 127.0.0.2
  acl Group all-of A2

Thus, the group (i.e., InnerNode) ACL destructor may access already deleted
children regardless of the global ACL deletion order (FIFO or LIFO with
respect to ACL creation). Instead of relying on the deletion order to protect
InnerNode, we remove the InnerNode ACL destructor completely and rely on a
global set of registered ACLs to destroy all ACLs.

The old code was destroying all explicit ACLs in the same centralized fashion.
We now add implicit ACLs (commonly used by InnerNodes) to the centralized
destruction sequence. We added a new destruction-dedicated container to avoid
messing with the by-name ACL search that Config.aclList global is used for.
This new container will become unnecessary once we start refcounting ACLs.

diff --git a/src/acl/Acl.cc b/src/acl/Acl.cc
index ebe87390f3599f12cb1e01ee5713bdc91da20595..1e925e7542f02f858a5b91727322f478de462cc1 100644 (file)
--- a/src/acl/Acl.cc
+++ b/src/acl/Acl.cc
@@ -31,6 +31,7 @@
 #include "squid.h"
 #include "acl/Acl.h"
 #include "acl/Checklist.h"
+#include "acl/Gadgets.h"
 #include "anyp/PortCfg.h"
 #include "cache_cf.h"
 #include "ConfigParser.h"
@@ -295,12 +296,13 @@ ACL::ParseAclLine(ConfigParser &parser, ACL ** head)
                A->cfgline);
     }
 
-    // prepend so that ACLs declared later (and possibly using earlier ACLs)
-    // are destroyed earlier (before the ACLs they use are destroyed)
+    // add to the global list for searching explicit ACLs by name
     assert(head && *head == Config.aclList);
-    A->registered = true;
     A->next = *head;
     *head = A;
+
+    // register for centralized cleanup
+    aclRegister(A);
 }
 
 bool

diff --git a/src/acl/Acl.h b/src/acl/Acl.h
index 9e71e397d9825a821de099cb3d190a5b33fa576b..9bcdc16833e76750c5deef84e829ab8be45552fb 100644 (file)
--- a/src/acl/Acl.h
+++ b/src/acl/Acl.h
@@ -138,7 +138,7 @@ public:
     char *cfgline;
     ACL *next; // XXX: remove or at least use refcounting
     ACLFlags flags; ///< The list of given ACL flags
-    bool registered; ///< added to Config.aclList and can be reused via by FindByName()
+    bool registered; ///< added to the global list of ACLs via aclRegister()
 
 public:
 

diff --git a/src/acl/Gadgets.cc b/src/acl/Gadgets.cc
index e4e3e6b3bbacd51224b1eb7f07206466cfadcf4f..c96dcf0e47fc3447e067535ff6ac02cc1e08145f 100644 (file)
--- a/src/acl/Gadgets.cc
+++ b/src/acl/Gadgets.cc
@@ -50,6 +50,14 @@
 #include "HttpRequest.h"
 #include "Mem.h"
 
+#include <set>
+#include <algorithm>
+
+
+typedef std::set<ACL*> AclSet;
+/// Accumulates all ACLs to facilitate their clean deletion despite reuse.
+static AclSet *RegisteredAcls; // TODO: Remove when ACLs are refcounted
+
 /* does name lookup, returns page_id */
 err_type
 aclGetDenyInfoPage(AclDenyInfoList ** head, const char *name, int redirect_allowed)
@@ -244,23 +252,38 @@ aclParseAclList(ConfigParser &, Acl::Tree **treep, const char *label)
     *treep = tree;
 }
 
+void
+aclRegister(ACL *acl)
+{
+    if (!acl->registered) {
+        if (!RegisteredAcls)
+            RegisteredAcls = new AclSet;
+        RegisteredAcls->insert(acl);
+        acl->registered = true;
+    }
+}
+
 /*********************/
 /* Destroy functions */
 /*********************/
 
+/// helper for RegisteredAcls cleanup
+static void
+aclDeleteOne(ACL *acl)
+{
+    delete acl;
+}
+
+/// called to delete ALL Acls.
 void
 aclDestroyAcls(ACL ** head)
 {
-    ACL *next = NULL;
-
-    debugs(28, 8, "aclDestroyACLs: invoked");
-
-    for (ACL *a = *head; a; a = next) {
-        next = a->next;
-        delete a;
+    *head = NULL; // Config.aclList
+    if (AclSet *acls = RegisteredAcls) {
+        debugs(28, 8, "deleting all " << acls->size() << " ACLs");
+        std::for_each(acls->begin(), acls->end(), &aclDeleteOne);
+        acls->clear();
     }
-
-    *head = NULL;
 }
 
 void

diff --git a/src/acl/Gadgets.h b/src/acl/Gadgets.h
index be68d3335f7acf69a6d7feb82eb69f5defaa29d9..4f21fe11b5ab6af0dd830b92f9bddb306ca7c157 100644 (file)
--- a/src/acl/Gadgets.h
+++ b/src/acl/Gadgets.h
@@ -13,6 +13,9 @@ class dlink_list;
 class StoreEntry;
 class wordlist;
 
+/// Register an ACL object for future deletion. Repeated registrations are OK.
+/// \ingroup ACLAPI
+void aclRegister(ACL *acl);
 /// \ingroup ACLAPI
 void aclDestroyAccessList(acl_access **list);
 /// \ingroup ACLAPI

diff --git a/src/acl/InnerNode.cc b/src/acl/InnerNode.cc
index fc5fa2ab8cb5878dea8300320d650e7e6d8fb290..c8042ecc5e749f772f3c9158eb30ec1c33117f75 100644 (file)
--- a/src/acl/InnerNode.cc
+++ b/src/acl/InnerNode.cc
@@ -10,22 +10,6 @@
 #include "wordlist.h"
 #include <algorithm>
 
-// "delete acl" class to use with std::for_each() in InnerNode::~InnerNode()
-class AclDeleter
-{
-public:
-    void operator()(ACL* acl) {
-        // Do not delete explicit ACLs; they are maintained by Config.aclList.
-        if (acl && !acl->registered)
-            delete acl;
-    }
-};
-
-Acl::InnerNode::~InnerNode()
-{
-    std::for_each(nodes.begin(), nodes.end(), AclDeleter());
-}
-
 void
 Acl::InnerNode::prepareForUse()
 {

diff --git a/src/acl/InnerNode.h b/src/acl/InnerNode.h
index bd20dd5d258d6525e3bccafd06bbe587be73c801..81865dbbe42a8427232f87d2f0c291e6f326901c 100644 (file)
--- a/src/acl/InnerNode.h
+++ b/src/acl/InnerNode.h
@@ -13,7 +13,7 @@ typedef std::vector<ACL*> Nodes; ///< a collection of nodes
 class InnerNode: public ACL
 {
 public:
-    virtual ~InnerNode();
+    // 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;