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

On 15/06/2014 4:44 a.m., Alex Rousskov wrote:
> 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.

The other options are POD-style options. They can be reset relatively
okay by the parsers memset() and are filled out by the config parse
routines.

SSL options are partially those type of options and partially SSL
library objects generated from those options. Generated options should
be in SquidConfig2 or one of the new standalone config gobals.

Also, the same set of config settings are needed by *_port, cache_peer,
and sslproxy_* directives. It makes sense to have one class type for SSL
config which is used by all three.

On the other side AnyP::PortCfgPointer already exists to reference count
the port objects. We could spend time converting the raw pointers to use
it instead so your patch here is not dangerous.

Amos
Received on Sun Jun 15 2014 - 06:02:48 MDT