Re: ACLFilledChecklist::checkCallback() not needed?

On 27/06/2012 4:17 p.m., Alex Rousskov wrote:
> Hello,
>
> Does anybody want to keep ACLFilledChecklist::checkCallback()? Long
> time ago, that method was used to unlock auth_user_request(s) via
> AUTHUSERREQUESTUNLOCK() to avoid memory leaks. Today, we use refcounting
> pointers for auth_user_requests so they should not leak even without an
> explicit NULL assignment.
>
> The method, specifically its questionable conn()->auth_user_request
> part, may be in the way of authentication because it results in
> forgetting connection's authentication state under some conditions. I
> would like to remove it. Any objections?

As I understand it this is about race conditions between validating the
credentials and reconfiguring Squid. For example; starting an
authentication lookup, then reconfiguring without auth helpers, then
resuming the checklist operation. Stateful auth lookups could hit this
badly as the validation period is so long and twisted.

That said, the UserRequest object is probably the better place to handle
such races by emitting a failed validation result if the auth system
goes away or breaks before completion.

IMO remove it and check/update UserRequest can handle the auth race
cleanly by itself. If we find other bugs later they need fixing via
other means.

Amos

>
> The ACLChecklist::checkCallback() will stay, of course, but it will no
> longer be virtual.
>
> I quoted current and the "original" code below for your convenience.
>
>
> Thank you,
>
> Alex.
>
> current code (that I would like to remove):
>
>> void
>> ACLFilledChecklist::checkCallback(allow_t answer)
>> {
>> debugs(28, 5, HERE << this << " answer=" << answer);
>>
>> #if USE_AUTH
>> /* During reconfigure, we can end up not finishing call
>> * sequences into the auth code */
>>
>> if (auth_user_request != NULL) {
>> /* the filled_checklist lock */
>> auth_user_request = NULL;
>> // It might have been connection based
>> // In the case of sslBump we need to preserve authentication info
>> // XXX: need to re-evaluate this. ACL tests should not be playing with
>> // XXX: wider scoped TCP connection state, even if the helper lookup i!
>> if (conn() && !conn()->switchedToHttps()) {
>> conn()->auth_user_request = NULL;
>> }
>> }
>> #endif
>>
>> ACLChecklist::checkCallback(answer); // may delete us
>> }
>
> which originated from the middle/unlocking part of this code (see trunk
> r9509.1.29):
>
>> void
>> ACLChecklist::checkCallback(allow_t answer)
>> {
>> PF *callback_;
>> void *cbdata_;
>> debugs(28, 3, "ACLChecklist::checkCallback: " << this << " answer=" << answe
>>
>> /* During reconfigure, we can end up not finishing call
>> * sequences into the auth code */
>>
>> if (auth_user_request) {
>> /* the checklist lock */
>> AUTHUSERREQUESTUNLOCK(auth_user_request, "ACLChecklist");
>> /* it might have been connection based */
>> assert(conn() != NULL);
>> /*
>> * DPW 2007-05-08
>> * yuck, this make me uncomfortable. why do this here?
>> * ConnStateData will do its own unlocking.
>> */
>> AUTHUSERREQUESTUNLOCK(conn()->auth_user_request, "conn via ACLChecklist"
>> conn()->auth_type = AUTH_BROKEN;
>> }
>>
>> callback_ = callback;
>> callback = NULL;
>>
>> if (cbdataReferenceValidDone(callback_data, &cbdata_))
>> callback_(answer, cbdata_);
>>
>> delete this;
>> }
Received on Wed Jun 27 2012 - 05:02:41 MDT