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.
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.
=== modified file 'src/acl/Acl.cc'
--- src/acl/Acl.cc 2013-12-02 00:35:50 +
+++ src/acl/Acl.cc 2013-12-10 23:48:56 +
@@ -14,40 +14,41 @@
* sources; see the CREDITS file for full details.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
*
*/
#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
#include Debug.h
#include dlink.h
#include globals.h
#include profiler/Profiler.h
#include SquidConfig.h
const ACLFlag ACLFlags::NoFlags[1] = {ACL_F_END};
const char *AclMatchedName = NULL;
bool ACLFlags::supported(const ACLFlag f) const
{
if (f == ACL_F_REGEX_CASE)
return true;
return (supported_.find(f) != std::string::npos);
}
@@ -278,46 +279,47 @@ 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);
}
