[PATCH] Destroy ACLs in the reverse order of creation

[Alex Rousskov]

Hello,

    The attached patch destroys ACLs in the reverse order of creation to
avoid destruction segfaults during reconfiguration. I could reproduce
segfaults in v3.3-based code. I saw access to the already destroyed ACL
memory in trunk; I suspect trunk did not segfault by luck as the bug
appears to be there.

Group ACLs created later may use other ACLs created earlier. A group ACL
must be deleted first so that its AclDeleter can safely access
registration status (and avoid double deletion) of the ACLs it uses.
Since ACLs are remembered (in Config.aclList) using a singly-linked
list, it is difficult to change their deletion order.  Instead, we
changed their listing order from FIFO to LIFO.

As far as I can tell, the ACL storage order is not important for the
rest of the code but please let me know if I missed any cases.


Thank you,

Alex.

Destroy ACLs in the reverse order of creation to avoid destruction segfaults
during reconfiguration.

Group ACLs created later may use other ACLs created earlier. A group ACL must
be deleted first so that its AclDeleter can safely access registration status
(and avoid double deletion) of the ACLs it uses. Since ACLs are remembered (in
Config.aclList) using a singly-linked list, it is difficult to change their
deletion order.  Instead, we change their listing order from FIFO to LIFO.

=== modified file 'src/acl/Acl.cc'
--- src/acl/Acl.cc	2013-08-29 09:21:53 +0000
+++ src/acl/Acl.cc	2013-11-28 23:56:07 +0000
@@ -278,47 +278,45 @@ ACL::ParseAclLine(ConfigParser &parser, 
     /*split the function here */
     A->parse();
 
     /*
      * Clear AclMatchedName from our temporary hack
      */
     AclMatchedName = NULL;	/* ugly */
 
     if (!new_acl)
         return;
 
     if (A->empty()) {
         debugs(28, DBG_CRITICAL, "Warning: empty ACL: " << A->cfgline);
     }
 
     if (!A->valid()) {
         fatalf("ERROR: Invalid ACL: %s\n",
                A->cfgline);
     }
 
-    /* append */
+    // prepend so that ACLs declared later (and possibly using earlier ACLs)
+    // are destroyed earlier (before the ACLs they use are destroyed)
     assert(head && *head == Config.aclList);
     A->registered = true;
-
-    while (*head)
-        head = &(*head)->next;
-
+    A->next = *head;
     *head = A;
 }
 
 bool
 ACL::isProxyAuth() const
 {
     return false;
 }
 
 /* ACL result caching routines */
 
 int
 ACL::matchForCache(ACLChecklist *checklist)
 {
     /* This is a fatal to ensure that cacheMatchAcl calls are _only_
      * made for supported acl types */
     fatal("aclCacheMatchAcl: unknown or unexpected ACL type");
     return 0;		/* NOTREACHED */
 }