Re: [PREVIEW] Various ACL fixes around matchAclList

On 05/30/2012 09:19 PM, Amos Jeffries wrote:
> On 31.05.2012 11:59, Alex Rousskov wrote:
>> Attached is the fourth version of the work-in-progress patch that
>> attempts to fix most of the bugs discussed in recent ACL-related
>> squid-dev threads ("DUNNO state and implicit ACLs" and "Handle ACLs that
>> are neither denied nor allowed") and other ACL bugs.

> Thank you. Just done a quick once-over to get my head around the
> changes, there is probably more.
>
>
> in src/acl/Checklist.cc:
>
> * You removed the currentAnswer(ACCESS_DUNNO) state reset at the start
> of parametered fachCheck(). This allows the answer from one check to
> potentially bleed over into following checks.

I am not sure what you mean when you talk about bleeding: Patched code
does not use a concept of "current answer". The final answer is set
exactly once, right before notifying the caller. Do you see some patched
code path that allows an old answer to affect a future check? If so,
that is a bug that should be fixed (but not by setting the "current"
answer to something in the beginning of each check, IMO).

> The parameterized and non-parameterised fastCheck() need to be kept in
> sync semantically. The only difference being that one uses the Checklist
> aclList and its result value, and the other uses the parameter and a
> hard-coded ACCESS_ALLOW result value on full match.

Another difference is that the void fastCheck has implicit rules while
the parameterized version cannot have them. At least that is my
understanding:

    # parameterized version:
    # foo is applied when all its ACLs match:
    option1 foo acl1 acl2 acl3 ...
    # bar is applied when all its ACLs match:
    option1 bar acl1 acl2 acl3 ...
    ...
    # (when this processing stops depends on the option)

    # void version:
    # baz is allowed when all its ACLs match (processing stops)
    option_baz allow acl1 acl2 acl3 ...
    # baz is allowed when all its ACLs match (processing stops)
    option_baz allow acl1 acl2 acl3 ...
    # otherwise, baz is denied (processing stops)

If you think this is wrong, please give an example of a squid.conf
option that uses parameterized fastCheck() with an implicit rule
semantics (i.e., a "do something special when the _last_ line does not
match" semantics rather than "ignore every line where ACLs do not match"
semantics).

> To set the final states cleanly and get some useful debugging around
> these tests, I think it should be:
>
> allow_t const &
> ACLChecklist::fastCheck(const ACLList * list)
> {
> PROF_start(aclCheckFast);
> + // assume DENY/ALLOW on mis/matches due to not having acl_access
> object
> + if (matchAclList(list, true))
> + markFinished(ACCESS_ALLOWED);
> + else
> + calcImplicitAnswer(ACCESS_ALLOWED);
> PROF_stop(aclCheckFast);
> return currentAnswer();
> }

The above would be wrong because the else clause overwrites an explicit
exceptional outcome (e.g., AUTH_REQUIRED) that could be set by some ACLs
by calling markFinished(). That outcome ought to be preserved for the
caller.

However, even if we fix that by adding a "!finished()" guard (as is done
in the patch), the result of calcImplicitAnswer(ACCESS_ALLOWED) is
equivalent to markFinished(ACCESS_DENIED) (which is what the patch
calls). So, technically, the outcome would be the same.

I agree that there is more debugging in calcImplicitAnswer(), but that
debugging would lie if applied to the parameterized fastCheck() because
there are no "allow" rules seen in that case. parameterized fastCheck()
should only be applied to rules without explicit allow/deny keywords.

If this is about debugging, I am happy to add more debugging to the
parameterized fastCheck(), of course. I was thinking that markFinished()
should get a second parameter to specify the "reason" for finishing the
checks. It would be handy in this case.

> in src/external_acl.cc
> * looks to be missing cbdataReferenceDone(ch->extacl_entry); when before
> the new entry=NULL

Is it missing after the following lines?

> entry = static_cast<external_acl_entry *>(hash_lookup(acl->def->cache, key));
>
> external_acl_entry *staleEntry = entry;
> if (entry && external_acl_entry_expired(acl->def, entry))
> entry = NULL;

There was no cbdataReferenceDone() in the similar area of the old code.
I am guessing that we need to unlock ch->extacl_entry only when we use
that entry. In the above code, the "entry" variable does not come from
ch->extacl_entry. It comes from cache: hash_lookup(acl->def->cache...).

Does this clarify? Am I looking at the wrong src/external_acl.cc code?

> * comment "no need to piggiback to a pending request if that request is
> sufficient" would be better written as:
> "background requests have no need to piggiback on a pending request"

Polished as:

> // A background refresh has no need to piggiback on a pending request:
> // When the pending request completes, the cache will be refreshed anyway.
> if (oldstate && inBackground) {
> debugs(82, 7, HERE << "'" << def->name << "' queue is already being ...
> return;
> }

(this avoids the implication that there is a "background request"
[queued] already when we have not created that request)

> in src/ident/AclIdent.cc:
> * IdentLookup::checkForAsync local variable ConnStateData *conn should
> probably be a const.

Fixed.

> * The new assert() seems like not such a good idea. Perhapse a "BUG"
> debugs() message and broken-lookup result instead of killing Squid.

I agree. I am using assert()s to ease debugging while the patch is being
tested (to get coredump/backtrace). Will replace with something less
drastic for the final version.

Thanks a lot for looking at this mess!

Alex.
Received on Thu May 31 2012 - 04:49:39 MDT