Centrally destroy all 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 7b7a42dcd2426e8e8f95cd9edf7134d7fc34d9fb..41b553860061d1efa098e331ad7974044771620a 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"
@@ -298,12 +299,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 c593434883dbece4afd705dc176bb187d97ac627..841438403f36ecb4ca72f2a9a7d9d87c7ff82c41 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 deb002496a13ca15ec879fac21f25f592fe7a27e..c35867532cb30fe6c7fc5ee1333eae323e3357c5 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 a5a36f000050daee5e592061d1265bfc383ae616..e3e75ea420561489da01633b3d7c6685b118c216 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;