[squid-dev] Broken trunk after r14735, r14726

[Amos Jeffries]

On 19/07/2016 6:58 a.m., Christos Tsantilas wrote:
> On 07/18/2016 08:32 PM, Alex Rousskov wrote:
>> On 07/18/2016 08:49 AM, Christos Tsantilas wrote:
>>> On 07/18/2016 02:12 PM, Christos Tsantilas wrote:
>>>> On 07/16/2016 03:56 PM, Amos Jeffries wrote:
>>>>> On 16/07/2016 7:02 a.m., Alex Rousskov wrote:
>>>>>> * After r14726 (GnuTLS: support for TLS session resume): Squid
>>>>>> segfaults
>>>>>> when attempting to connect to a Secure ICAP service. Official Squid
>>>>>> v4.0.12 suffers from this bug.
>>
>>>> It is a strange crash. Is it a corrupted SSL object?
>>
>> It is an already freed object (its references member is 0).
>>
>>
>>> The patch uses the following line:
>>>   Security::GetSessionResumeData(Security::SessionPointer(ssl),
>>> icapService->sslSession);
>>>
>>>
>>> The Security::SessionPointer, is a LockingPointer which in trunk-r14726,
>>> does not increase the references of the attached "SSL" object in
>>> constructor.
>>> So the SSL will be released after the Security::SessionPointer is
>>> destroyed, immediately after the above line executed.
>>
>> ... which would explain the zero "references" value. This raises a
>> question -- did the old code work because the old LockingPointer
>> constructor was locking? If the answer is yes, then the
>> invisible-in-callers change from locking to non-locking was a mistake.
> 
> The old LockingPointer constructor was not locking.

The old resume code worked because Squid was passing around and storing
a raw-pointer not using any LockingPointer, even for a portion of its
lifecycle.

> This was because most(?) of openSSL functions return pre-locked objects
> 
> But looks that the best to avoid the confusion is to allow only methods
> resetAndLock and resetWithoutLock
> 

Dropping the non-locking constructor and forcing explicit resetFoo() is
probably for the best. Though it would not have helped in this case. I
did think the 'ssl' variable being passed in there was pre-locked so
would have coded as resetWithoutLocking().
 Would that difference in syntax have helped in the (brief) review?


>>
>> I can only repeat my earlier suggestions to hide that dangerous
>> constructor behind an explicit static method like
>> LockingPointer::NewWithoutLocking() and to assert that
>> resetWithoutLocking() method is fed a previously locked object.
> 
> something like that.

Nod. I'm about to try a build with dropped construtor to see what
breaks. That should highlight if a builder function is actually
necessary. I hope we can avoid it.


PS. Alex is the other r14735 issue still present in current trunk now
that r14726 is reverted?

Amos