Re: [PATCH 5/8] reconfiguration leaks: objects tied to http_port

On 06/13/2014 07:25 PM, Amos Jeffries wrote:
> On 14/06/2014 8:07 a.m., Alex Rousskov wrote:
>> On 04/25/2014 02:59 AM, Amos Jeffries wrote:
>>> On 25/04/2014 12:55 p.m., Alex Rousskov wrote:
>>>> Do not leak [SSL] objects tied to http_port and https_port on reconfigure.
>>>>
>>>> PortCfg objects were not destroyed at all (no delete call) and were
>>>> incorrectly stored (excessive cbdata locking). This change adds
>>>> destruction and removes excessive locking to allow the destructed
>>>> object to be freed. It also cleans up forgotten(?) clientca and crlfile
>>>> PortCfg members.
>>>>
>>>> This change fixes a serious leak but also carries an elevated risk:
>>>> There is a lot of code throughout Squid that does not check the pointers
>>>> to the objects that are now properly destroyed. It is possible that some
>>>> of that code will crash some time after reconfigure. It is not possible
>>>> to ensure that this does not happen without rewriting/fixing the
>>>> offending code to use refcounting. Such a rewrite would be a relatively
>>>> large change outside this patch scope. We may decide that it is better
>>>> to leak than to take this additional risk.
>>>>
>>>> Alex.
>>>>
>>>
>>> -0.
>>>
>>> I have a patch moving the SSL config options into a standalone
>>> ref-counted object. That can be polished up and references added to each
>>> ConnStateData fairly easily.
>>
>> Amos, what is the status of that patch? Any ETA? Do you expect your
>> changes to be easily portable to v3.3?
>
> Stalled behind the larger works. If it is urgent I can did it out and
> polish it up.
>
> It could be back-ported to 3.3 if you like. The design is a new
> Ref-Countable class to hold all the SSL options (and generated state)
> leaving just a Pointer to it in the main config class.
> * Ports which needed a clone operation took a copy of the pointer and
> share the context.
> * client/server context initialization functions take a Pointer to the
> class and update its state content.

Why treat SSL options differently from all other port options? Should
not the whole PortCfg object be refcounted instead?

Please note that if you refcount the SSL options and do nothing else,
the leak will not be fixed. The SSL options (and other things) will
continue to leak because the leaking port structure will keep pointing
to them. Thus, as far as I can see, the patch you described will need
the proposed leak fix anyway.

Cheers,

Alex.
Received on Sat Jun 14 2014 - 16:45:07 MDT