[PATCH] Destroy ACLs properly, take2

On 12/01/2013 05:43 PM, Alex Rousskov wrote:

> The attached patch destroys ACLs in the reverse order of creation to
> avoid destruction segfaults during reconfiguration.

> Done as trunk r13165.

Sorry, that was not enough. I somehow missed an obvious use case that
the committed fix does not cover, despite specifically trying to imagine
it during testing: It is actually possible for a group ACL G to use an
ACL A1 that was created before G and an ACL A2 that was created after G:

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

Such order of ACL creation makes any creation-based destruction order
invalid: Whether we use FIFO (as before) or LIFO (as after r13165), G's
destructor may dump core when deleting either A1 or A2 because one of
them will be already deleted earlier via Config.aclList.

The attached patch fixes the problem differently. Instead of using
Config.aclList to register and destroy explicit ACLs and then relying on
InnerNode destructor to destroy its sometimes explicit children, the
take2 patch dedicates a new container to register and destroy both
implicit and explicit ACLs in one sweep.

The InnerNode destructor is gone now, with aclDestroyAcls() replacing
its functionality.

The Config.aclList global is still used for searching explicit ACLs by
name -- no changes there.

Refcounting remains the preferred long-term solution, of course. I have
once again reviewed the changes required to make that happen, and can
confirm that they are too big for me to volunteer for that project right
now. Volunteers welcome.

Thank you,

Alex.
P.S. I left the registration order as in r13165 because it is much
faster (eliminates one linear search through all ACLs when adding a new
ACL). That can be reverted if we find a reason to search starting with
older ACLs. However, if that search order is important, we should
replace Config.aclList linked list with an ordered-by-ACL-name
container, to eliminate another case of a linear search.