Re: [PREVIEW] Various ACL fixes around matchAclList

On 05/31/2012 03:37 AM, Amos Jeffries wrote:
>>> 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.

In the patched code, it is not possible to set finished without setting
an ACCESS_* value: The only way to make finished_ true is to call
markFinished() and that method requires the [final] answer parameter.
This was done on purpose, to fix unpatched code bugs where the "initial"
or "current" answer accidentally becomes final.

It is also not possible to call markFinished() twice during a single check.

I now further restricted this by setting finished_ to false in the
beginning of each check rather than in ACLChecklist::matchNodes(). This
will make the patched code look more like the old one ("first reset then
check") while maintaining stricter state guarantees. See the resurrected
preCheck() method in the attached patch.

> All the
> other check functions set their initial values explicitly and then run.

Not in the patched code. There is no concept of "initial" answer in the
patched code.

I did not want to rename currentAnswer() in the patch because that would
cause a lot of non-changes. If the final patch is approved, I will
rename currentAnswer() to finalAnswer() as a separate commit.

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

I agree that the choice between calcImplicitAnswer(ACCESS_ALLOWED) and
markFinished(ACCESS_DENIED) was not clear. I rewrote source code
comments to the fastCheck() declarations to clarify this.

While rewriting those comments, I realized that it is actually wrong to
try to keep the two fastCheck() methods symmetric or similar! These
methods are _not_ a slight variation of the same concept. They actually
work on two very different levels of ACL checking (a single list versus
a list of lists) and must have different defaults. Using math jargon,
they are like a single sum and a single multiplication [of sums] -- two
rather different concepts!

I hope the new method declaration comments clarify that.

I believe naming these two very different methods the same was a mistake
(which we can fix later).

In general, while no changes can guarantee that things will not become
worse later, I believe the patched code is much less susceptible to
accidental breakage in this area. The current APIs are so fuzzy that
"let's tweak it here and hope for the best" was apparently the norm for
a while. Patched APIs are more strict and better documented, reducing
the probability that things will deteriorate again.

What I am less sure about is whether the patched code satisfies all the
unwritten conventions and delivers previously relied upon functionality
in all [corner] cases. I am pretty sure the patched code tries to do the
right thing, but I suspect that is not what some admins expect after
years of doing something fuzzy.

Attached is take5 of the patch with all the changes promised so far.
AFAICT, there are at lest two areas were we may not be in agreement:

1) Whether current state initialization before each check is sufficient.
I hope take5 changes and the above comments will convince you that it is.

2) Whether parameterized fastCheck() declaration comments are correct.

Thank you,

Alex.