[squid-dev] checklist->conn assertion in DestinationIp.cc

[Alex Rousskov]

On 07/17/2015 11:31 PM, Amos Jeffries wrote:
> On 18/07/2015 6:09 a.m., Alex Rousskov wrote:
>>     Intercepting Squids sometimes fail with the following assertion in
>> ACLDestinationIP::match():
>>
>>>     if (Config.onoff.client_dst_passthru && ... intercepted ...) {
>>>         assert(checklist->conn() && checklist->conn()->clientConnection != NULL);
>>>         return ACLIP::match(checklist->conn()->clientConnection->local);
>>>     }
>>
>> There are several reports about these failures on squid-users, including
>> http://lists.squid-cache.org/pipermail/squid-users/2015-May/003562.html
>>
>> The assertion makes no sense to me -- why would an ACL assert that a
>> connection is valid? A lot of things can happen between the time the ACL
>> checklist was formed and the time the ACL got evaluated. This is true
>> for all ACLs, but should be especially obvious for slow/asynchronous
>> ACLs such as "dst".


> What is a transaction without any existing state objects?

A finished/gone transaction?


> The connection may be closed (definition of 'invalid'?) and still pass
> this test, so long as the state actually exists.

Evidently, the state does not exist. Judging by the number of Squid if
statements checking similar conditions, there are many situations where
the asserted condition may be false. On the other hand, it is possible
that this ACL code is exceptional or that all those other checks are in
vain, of course. That is why I am trying to understand why the above
assertion was postulated...


> If the ConnStateData and ConnStateData::clientConnection are previously
> deleted and things are still starting ACL tests something very bad has
> happened. Probably broken refcount/cbdata locking on the Job-like object
> holding the Checklist.

AFAICT, many Checklists are not locked by Jobs. For clear examples, see
ConnStateData::postHttpsAccept() and startPeekAndSpliceDone(), but I
suspect the vast majority of nonBlockingCheck() calls do not lock
Checklist either, despite using class members (instead of local
variables) to keep the Checklist pointer.

Please note that the asserting ACL check was _resumed_ in this case
(probably after an async DNS lookup), not started from the beginning.
Since async actions are always a possibility, I do not think we can
assert anything about the transaction state at the ACL check time.


>> Is suggest replacing the assert with an if-statement to return -1
>> (matching failure) when the connection is gone. Rationale: With the
>> connection gone, the matching result probably does not matter anymore so
>> there is little incentive for us to use alternative (and insecure!)
>> sources of destination information.
>>
>> Any better ideas?


> That would seem to work for this problem. But be careful about other
> hidden side effects of trying to complete delivery of a client response
> without any existing client state object(s).

AFAICT, ACLChecklist::checkCallback() checks for callback validity
before delivering the answer to that callback.


> CbcPointer use for FilledChecklist::conn_ should also get a look-in to
> see if its possible. The hitch there IIRC, was that CbcPointer get()
> still produces NULL if the object has been 'deleted' but locks are still
> existing for the Checklist to be destructed.

I agree that FilledChecklist::conn_ should use a CbcPointer to make its
dereference safer, but a simple replacement would probably just trigger
more [harder-to-triage] assertions because of the problem discussed below.

Virtually any existing FilledChecklist::conn()-dereferencing ACL code
that simply checks whether the pointer is NULL before dereferencing it
is a ticking time bomb -- the conn_ pointer returned by conn() may be
not NULL, but the corresponding ConnStateData destructor has been called
already. As you know, cbdata only protects raw memory from being
deleted, not the destructor from being called...


How about the following two changes?

1. Change FilledChecklist::conn() to return NULL when conn_ is invalid.
2. Change the above assertion into an if statement, returning -1.


If, after the above is done, somebody volunteers to replace the raw
conn_ pointer with a CbcPointer, they should be welcomed to do that, of
course.


Thank you,

Alex.