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 _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev