Hello,
The attached patch prevents asserts when Squid reuses the same
Checklist object for multiple ACL checks. I missed one use case when
adding Checklist reuse controls for trunk r12859 (Boolean ACLs). The bug
can be triggered by a combination of log_access and access_log ACLs, for
example.
Concurrent checks are not supported, but it is possible for the same
ACLChecklist to be used for a sequence of checks, alternating
fastCheck(void) and fastCheck(list) calls. We needed a
different/dedicated mechanism to detect check concurrency (added
ACLChecklist::occupied_), and we needed to preserve (and then restore)
pre-set accessList during fastCheck(list) checks.
HTH,
Alex.
Fix detection of concurrent ACLChecklist checks, avoiding !accessList asserts.
Concurrent checks are not supported, but it is possible for the same
ACLChecklist to be used for a sequence of checks, alternating fastCheck(void)
and fastCheck(list) calls. We needed a different/dedicated mechanism to detect
check concurrency (added ACLChecklist::occupied_), and we needed to preserve
(and then restore) pre-set accessList during fastCheck(list) checks.
=== modified file 'src/acl/Checklist.cc'
--- src/acl/Checklist.cc 2013-06-01 10:01:13 +0000
+++ src/acl/Checklist.cc 2013-06-07 16:47:47 +0000
@@ -43,40 +43,45 @@
calcImplicitAnswer();
cbdataReferenceDone(accessList);
checkCallback(currentAnswer());
}
void
ACLChecklist::markFinished(const allow_t &finalAnswer, const char *reason)
{
assert (!finished() && !asyncInProgress());
finished_ = true;
allow_ = finalAnswer;
debugs(28, 3, HERE << this << " answer " << allow_ << " for " << reason);
}
/// Called first (and once) by all checks to initialize their state
void
ACLChecklist::preCheck(const char *what)
{
debugs(28, 3, HERE << this << " checking " << what);
+
+ // concurrent checks using the same Checklist are not supported
+ assert(!occupied_);
+ occupied_ = true;
+
AclMatchedName = NULL;
finished_ = false;
}
bool
ACLChecklist::matchChild(const Acl::InnerNode *current, Acl::Nodes::const_iterator pos, const ACL *child)
{
assert(current && child);
// Remember the current tree location to prevent "async loop" cases where
// the same child node wants to go async more than once.
matchLoc_ = Breadcrumb(current, pos);
// if there are any breadcrumbs left, then follow them on the way down
bool result = false;
if (matchPath.empty()) {
result = child->matches(this);
} else {
const Breadcrumb top(matchPath.top());
assert(child == top.parent);
@@ -131,48 +136,52 @@
// yes, we must pause until the async callback calls resumeNonBlockingCheck
asyncStage_ = asyncRunning;
return true;
}
// ACLFilledChecklist overwrites this to unclock something before we
// "delete this"
void
ACLChecklist::checkCallback(allow_t answer)
{
ACLCB *callback_;
void *cbdata_;
debugs(28, 3, "ACLChecklist::checkCallback: " << this << " answer=" << answer);
callback_ = callback;
callback = NULL;
if (cbdataReferenceValidDone(callback_data, &cbdata_))
callback_(answer, cbdata_);
+ // not really meaningful just before delete, but here for completeness sake
+ occupied_ = false;
+
delete this;
}
ACLChecklist::ACLChecklist() :
accessList (NULL),
callback (NULL),
callback_data (NULL),
asyncCaller_(false),
+ occupied_(false),
finished_(false),
allow_(ACCESS_DENIED),
asyncStage_(asyncNone),
state_(NullState::Instance())
{
}
ACLChecklist::~ACLChecklist()
{
assert (!asyncInProgress());
cbdataReferenceDone(accessList);
debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this);
}
ACLChecklist::NullState *
ACLChecklist::NullState::Instance()
{
return &_instance;
@@ -270,87 +279,91 @@
if (matchPath.empty()) {
result = accessList->matches(this);
} else {
const Breadcrumb top(matchPath.top());
matchPath.pop();
result = top.parent->resumeMatchingAt(this, top.position);
}
if (result) // the entire tree matched
markFinished(accessList->winningAction(), "match");
}
allow_t const &
ACLChecklist::fastCheck(const Acl::Tree * list)
{
PROF_start(aclCheckFast);
preCheck("fast ACLs");
asyncCaller_ = false;
- // This call is not compatible with a pre-set accessList because we cannot
- // tell whether this Checklist is used by some other concurent call, which
- // is not supported.
- assert(!accessList);
+ // Concurrent checks are not supported, but sequential checks are, and they
+ // may use a mixture of fastCheck(void) and fastCheck(list) calls.
+ const Acl::Tree * const savedList = accessList;
+
accessList = cbdataReference(list);
// assume DENY/ALLOW on mis/matches due to action-free accessList
// matchAndFinish() takes care of the ALLOW case
if (accessList && cbdataReferenceValid(accessList))
matchAndFinish(); // calls markFinished() on success
if (!finished())
markFinished(ACCESS_DENIED, "ACLs failed to match");
cbdataReferenceDone(accessList);
+ accessList = savedList;
+ occupied_ = false;
PROF_stop(aclCheckFast);
return currentAnswer();
}
/* Warning: do not cbdata lock this here - it
* may be static or on the stack
*/
allow_t const &
ACLChecklist::fastCheck()
{
PROF_start(aclCheckFast);
preCheck("fast rules");
asyncCaller_ = false;
debugs(28, 5, "aclCheckFast: list: " << accessList);
const Acl::Tree *acl = cbdataReference(accessList);
if (acl != NULL && cbdataReferenceValid(acl)) {
matchAndFinish(); // calls markFinished() on success
// if finished (on a match or in exceptional cases), stop
if (finished()) {
cbdataReferenceDone(acl);
+ occupied_ = false;
PROF_stop(aclCheckFast);
return currentAnswer();
}
// fall through for mismatch handling
}
// There were no rules to match or no rules matched
calcImplicitAnswer();
cbdataReferenceDone(acl);
+ occupied_ = false;
PROF_stop(aclCheckFast);
return currentAnswer();
}
/// When no rules matched, the answer is the inversion of the last rule
/// action (or ACCESS_DUNNO if the reversal is not possible).
void
ACLChecklist::calcImplicitAnswer()
{
const allow_t lastAction = (accessList && cbdataReferenceValid(accessList)) ?
accessList->lastAction() : allow_t(ACCESS_DUNNO);
allow_t implicitRuleAnswer = ACCESS_DUNNO;
if (lastAction == ACCESS_DENIED) // reverse last seen "deny"
implicitRuleAnswer = ACCESS_ALLOWED;
else if (lastAction == ACCESS_ALLOWED) // reverse last seen "allow"
implicitRuleAnswer = ACCESS_DENIED;
// else we saw no rules and will respond with ACCESS_DUNNO
debugs(28, 3, HERE << this << " NO match found, last action " <<
=== modified file 'src/acl/Checklist.h'
--- src/acl/Checklist.h 2013-05-28 14:28:15 +0000
+++ src/acl/Checklist.h 2013-06-07 16:47:47 +0000
@@ -207,36 +207,37 @@
Breadcrumb(): parent(NULL) {}
Breadcrumb(const Acl::InnerNode *aParent, Acl::Nodes::const_iterator aPos): parent(aParent), position(aPos) {}
bool operator ==(const Breadcrumb &b) const { return parent == b.parent && (!parent || position == b.position); }
bool operator !=(const Breadcrumb &b) const { return !this->operator ==(b); }
void clear() { parent = NULL; }
const Acl::InnerNode *parent; ///< intermediate node in the ACL tree
Acl::Nodes::const_iterator position; ///< child position inside parent
};
/// possible outcomes when trying to match a single ACL node in a list
typedef enum { nmrMatch, nmrMismatch, nmrFinished, nmrNeedsAsync }
NodeMatchingResult;
/// prepare for checking ACLs; called once per check
void preCheck(const char *what);
bool prepNonBlocking();
void completeNonBlocking();
void calcImplicitAnswer();
bool asyncCaller_; ///< whether the caller supports async/slow ACLs
+ bool occupied_; ///< whether a check (fast or non-blocking) is in progress
bool finished_;
allow_t allow_;
enum AsyncStage { asyncNone, asyncStarting, asyncRunning, asyncFailed };
AsyncStage asyncStage_;
AsyncState *state_;
Breadcrumb matchLoc_; ///< location of the node running matches() now
Breadcrumb asyncLoc_; ///< currentNode_ that called goAsync()
bool callerGone();
/// suspended (due to an async lookup) matches() in the ACL tree
std::stack<Breadcrumb> matchPath;
};
#endif /* SQUID_ACLCHECKLIST_H */
