[squid-dev] Broken trunk after r14735, r14726

[Christos Tsantilas]

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

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



>
>
> Thank you,
>
> Alex.
>