Re: [PREVIEW] Various ACL fixes around matchAclList

On 31/05/2012 4:49 p.m., Alex Rousskov wrote:
> 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 new version of fastCheck(list) is returning currentAnswer(); but not
ensuring that its set to anything sane before starting the match
testing. markFinished() from a previous testing run can set anything and
this run will potentially use it for the non-ALLOW event. All that needs
is a match to set finished somehow without a new ACCESS_* value. All the
other check functions set their initial values explicitly and then run.

>
> 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.

Technically, but its not clear and if/when someobody else plays with the
code things might change for the worse again.

>
>> 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?

Ah, right.

Amos
Received on Thu May 31 2012 - 09:37:35 MDT