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 +0000
+++ src/acl/Acl.cc 2013-12-10 23:48:56 +0000
@@ -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);
}
if (!A->valid()) {
fatalf("ERROR: Invalid ACL: %s\n",
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
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 */
}
/*
=== modified file 'src/acl/Acl.h'
--- src/acl/Acl.h 2013-12-02 00:36:24 +0000
+++ src/acl/Acl.h 2013-12-10 23:48:56 +0000
@@ -120,41 +120,41 @@ public:
virtual ACL *clone()const = 0;
/// parses node represenation in squid.conf; dies on failures
virtual void parse() = 0;
virtual char const *typeString() const = 0;
virtual bool isProxyAuth() const;
virtual wordlist *dump() const = 0;
virtual bool empty () const = 0;
virtual bool valid () const;
int cacheMatchAcl(dlink_list * cache, ACLChecklist *);
virtual int matchForCache(ACLChecklist *checklist);
virtual void prepareForUse() {}
char name[ACL_NAME_SZ];
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:
class Prototype
{
public:
Prototype ();
Prototype (ACL const *, char const *);
~Prototype();
static bool Registered(char const *);
static ACL *Factory (char const *);
private:
ACL const*prototype;
char const *typeString;
private:
static Vector<Prototype const *> * Registry;
static void *Initialized;
=== modified file 'src/acl/Gadgets.cc'
--- src/acl/Gadgets.cc 2013-11-29 23:44:28 +0000
+++ src/acl/Gadgets.cc 2013-12-10 23:48:56 +0000
@@ -33,40 +33,48 @@
* 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/AclDenyInfoList.h"
#include "acl/AclNameList.h"
#include "acl/Checklist.h"
#include "acl/Gadgets.h"
#include "acl/Strategised.h"
#include "acl/Tree.h"
#include "ConfigParser.h"
#include "errorpage.h"
#include "globals.h"
#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)
{
if (!name) {
debugs(28, 3, "ERR_NONE due to a NULL name");
return ERR_NONE;
}
AclDenyInfoList *A = NULL;
debugs(28, 8, HERE << "got called for " << name);
for (A = *head; A; A = A->next) {
AclNameList *L = NULL;
if (!redirect_allowed && strchr(A->err_page_name, ':') ) {
debugs(28, 8, HERE << "Skip '" << A->err_page_name << "' 30x redirects not allowed as response here.");
continue;
}
@@ -227,57 +235,72 @@ aclParseAclList(ConfigParser &, Acl::Tre
Acl::AndNode *rule = new Acl::AndNode;
rule->context(ctxLine.content(), config_input_line);
rule->lineParse();
MemBuf ctxTree;
ctxTree.init();
ctxTree.Printf("%s %s", cfg_directive, label);
ctxTree.terminate();
// We want a cbdata-protected Tree (despite giving it only one child node).
Acl::Tree *tree = new Acl::Tree;
tree->add(rule);
tree->context(ctxTree.content(), config_input_line);
assert(treep);
assert(!*treep);
*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
aclDestroyAclList(ACLList **list)
{
debugs(28, 8, "aclDestroyAclList: invoked");
assert(list);
cbdataFree(*list);
}
void
aclDestroyAccessList(acl_access ** list)
{
assert(list);
if (*list)
debugs(28, 3, "destroying: " << *list << ' ' << (*list)->name);
cbdataFree(*list);
}
/* m...@space.net (06.09.1996)
=== modified file 'src/acl/Gadgets.h'
--- src/acl/Gadgets.h 2013-10-25 00:13:46 +0000
+++ src/acl/Gadgets.h 2013-12-10 23:48:56 +0000
@@ -1,35 +1,38 @@
#ifndef SQUID_ACL_GADGETS_H
#define SQUID_ACL_GADGETS_H
#include "acl/forward.h"
#include "err_type.h"
#if HAVE_SSTREAM
#include <sstream>
#endif
class ConfigParser;
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
void aclDestroyAcls(ACL **);
/// \ingroup ACLAPI
void aclDestroyAclList(ACLList **);
/// Parses a single line of a "action followed by acls" directive (e.g., http_access).
/// \ingroup ACLAPI
void aclParseAccessLine(const char *directive, ConfigParser &parser, Acl::Tree **);
/// Parses a single line of a "some context followed by acls" directive (e.g., note n v).
/// The label parameter identifies the context (for debugging).
/// \ingroup ACLAPI
void aclParseAclList(ConfigParser &parser, Acl::Tree **, const char *label);
/// Template to convert various context lables to strings. \ingroup ACLAPI
template <class Any>
inline
void aclParseAclList(ConfigParser &parser, Acl::Tree **tree, const Any any)
{
std::ostringstream buf;
buf << any;
=== modified file 'src/acl/InnerNode.cc'
--- src/acl/InnerNode.cc 2013-05-28 22:33:15 +0000
+++ src/acl/InnerNode.cc 2013-12-10 23:48:56 +0000
@@ -1,48 +1,32 @@
#include "squid.h"
#include "acl/Acl.h"
#include "acl/BoolOps.h"
#include "acl/Checklist.h"
#include "acl/InnerNode.h"
#include "cache_cf.h"
#include "ConfigParser.h"
#include "Debug.h"
#include "globals.h"
#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()
{
std::for_each(nodes.begin(), nodes.end(), std::mem_fun(&ACL::prepareForUse));
}
bool
Acl::InnerNode::empty() const
{
return nodes.empty();
}
void
Acl::InnerNode::add(ACL *node)
{
assert(node != NULL);
nodes.push_back(node);
}
// one call parses one "acl name acltype name1 name2 ..." line
=== modified file 'src/acl/InnerNode.h'
--- src/acl/InnerNode.h 2013-12-02 00:36:24 +0000
+++ src/acl/InnerNode.h 2013-12-10 23:48:56 +0000
@@ -1,36 +1,36 @@
#ifndef SQUID_ACL_INNER_NODE_H
#define SQUID_ACL_INNER_NODE_H
#include "acl/Acl.h"
#include <vector>
namespace Acl
{
typedef std::vector<ACL*> Nodes; ///< a collection of nodes
/// An intermediate ACL tree node. Manages a collection of child tree 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;
/// the number of children nodes
Nodes::size_type childrenCount() const { return nodes.size(); }
/* ACL API */
virtual void prepareForUse();
virtual bool empty() const;
virtual wordlist *dump() const;
/// parses one "acl name type acl1 acl2..." line, appending to nodes
void lineParse();
/// appends the node to the collection and takes control over it
void add(ACL *node);
protected:
/// checks whether the nodes match, starting with the given one
