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 */