From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Mon, 6 Jan 2014 20:55:13 +0000 (-0700)
Subject: Centrally destroy all explicit and implicit ACLs to avoid destruction segfaults
X-Git-Tag: SQUID_3_5_0_1~435
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ed898bdf9bc9380f04bdf546bab761f8f1d8739f;p=thirdparty%2Fsquid.git

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 ebe87390f3..1e925e7542 100644
--- 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 9e71e397d9..9bcdc16833 100644
--- 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 e4e3e6b3bb..c96dcf0e47 100644
--- 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 be68d3335f..4f21fe11b5 100644
--- 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 fc5fa2ab8c..c8042ecc5e 100644
--- 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 bd20dd5d25..81865dbbe4 100644
--- 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;