Re: [PATCH] Destroy ACLs properly, take2

On 12/10/2013 05:24 PM, Alex Rousskov wrote:
> 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.

Committed to trunk as r13210.

Alex.
Received on Mon Jan 06 2014 - 21:00:20 MST