Hi all,
The problem has been discussed here:
http://www.squid-cache.org/mail-archive/squid-dev/201105/0116.html
This patch add the method ACLChecklist::reusableFastCheck which is
similar to the ACLChecklist::fastCheck method but it can be called many
times, and each time called a new independed access list check done.
Also I found some problems in ACLChecklist::fastCheck which the current
patch does not try to solve them, but we may take care of.
1) Documentation of ::fastCheck does not match implementation
--------------------------------------------------------------
Currently the documentation of the ACLChecklist::fastCheck does not
match the implementation. From the documentation:
* If there is no access list to check the default is to return DENIED.
...
* \retval 1/true Access Allowed
* \retval 0/false Access Denied
But in the case no access list exist (accessList = NULL) we have inside
::fastCheck():
currentAnswer(ACCESS_DENIED);
while (accessList) {
....
}
return currentAnswer() == ACCESS_DENIED;
In the case no access list defined, it return true ("access allowed").
We should fix the ::fastCheck or its documentation.
2) Bug in ::fastCheck() method
-------------------------------
Now, when the ::fastCheck() called for a second time or third time, the
accessList is NULL always:
- if not match because list exhausted in first call
- if match because of the "cbdataReferenceDone(accessList);" line
(src/acl/Checklist.cc line 350).
So a second call of the ::fastCheck() will return access allowed, even
if the initial decision was access denied...
This is not normal (I think it is a bug).
I propose to replace the old ::fastCheck() method with the new
::reusableFastCheck() code so the fastCheck() calls have simpler
semantics and are always reusable.
=== modified file 'src/acl/Checklist.cc'
--- src/acl/Checklist.cc 2009-03-08 19:41:27 +0000
+++ src/acl/Checklist.cc 2011-06-10 13:56:17 +0000
@@ -369,6 +369,29 @@
return currentAnswer() == ACCESS_DENIED;
}
+bool
+ACLChecklist::reusableFastCheck()
+{
+ currentAnswer(ACCESS_DENIED);
+ debugs(28, 5, "reusableCheckFast: list: " << accessList);
+ const acl_access *acl = cbdataReference(accessList);
+ while (acl != NULL && cbdataReferenceValid(acl)) {
+ preCheck();
+ if (matchAclListFast(acl->aclList)) {
+ cbdataReferenceDone(acl);
+ return currentAnswer() == ACCESS_ALLOWED;
+ }
+
+ /*
+ * Reference the next access entry
+ */
+ const acl_access *A = acl;
+ acl = cbdataReference(acl->next);
+ cbdataReferenceDone(A);
+ }
+ debugs(28, 5, "reusableCheckFast: no matches, returning: " << (currentAnswer() == ACCESS_DENIED));
+ return currentAnswer() == ACCESS_DENIED;
+}
bool
ACLChecklist::checking() const
=== modified file 'src/acl/Checklist.h'
--- src/acl/Checklist.h 2009-04-07 13:51:57 +0000
+++ src/acl/Checklist.h 2011-06-10 12:42:35 +0000
@@ -113,6 +113,14 @@
int fastCheck();
/**
+ * Trigger a blocking access check for a given access list
+ * The behavior and the result is similar to the ::fastCheck method,
+ * but this method does not change the internal state of the ACLChecklist
+ * object and each execution run an indepeneded access list check.
+ */
+ bool reusableFastCheck();
+
+ /**
* Trigger a blocking access check for a single ACL line (a AND b AND c).
*
* ACLs which cannot be satisfied directly from available data are ignored.
=== modified file 'src/ssl/support.cc'
--- src/ssl/support.cc 2011-05-13 07:59:19 +0000
+++ src/ssl/support.cc 2011-06-10 16:09:21 +0000
@@ -243,7 +243,7 @@
}
if (!ok && check) {
- if (check->fastCheck()) {
+ if (check->reusableFastCheck()) {
debugs(83, 3, "bypassing SSL error " << ctx->error << " in " << buffer);
ok = 1;
} else {
