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

On 17/06/2014 9:15 a.m., Alex Rousskov wrote:
> On 06/15/2014 12:02 AM, Amos Jeffries wrote:
>> 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.
>
>
>>>>>> -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?
>
>
>>>> Stalled behind the larger works. If it is urgent I can did it out and
>>>> polish it up.
>>>>
>>>> [...] 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?
>
>
>> 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.
>
> AFAICT, not all other options are "POD-style options that can be reset
> relatively okay by memset()" unless "relatively OK" means "small leaks
> are OK". Some non-SSL port options require specialized cleanup just like
> some SSL port options do (PortCfg::name and defaultsite are two examples
> with listenConn arguably being the third). Thus, the requirement for
> specialized cleanup is not a reason to treat SSL options differently
> from all other port options.
>
>
>> 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.
>
> While it may indeed be a good idea to group related options together,
> such grouping is irrelevant for the purpose of this discussion. Whether
> we group SSL-related options or not, they and other port options will
> continue to leak. They leak not because they are not grouped but because
> Squid leaks the Port structure they belong to (grouped or not).
>
>
>> 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.
>
> PortCfgPointer is not a reference counting pointer.

There is no remaining reason for that since we converted the TcpAcceptor
to emitting MasterXaction. The PortCfg pointer is not actually passed as
a parameter anywhere. Just one buggy piece of code which should have
been implemented differently.

<snip>
>
> Most likely, we should use the refcounting API for port pointers. Until
> that (or a better) solution is implemented, we should either
>

The attached (rough) patch converts the PortCfgPointer to reference
counted and fixes all parsing errors resulting from the change. Most of
the issues were due to use of raw-pointers and explicit
cbdataReference*() API.

Still have to add stubs to fix make check linkage errors and do some run
testing.

> * stop port leaks or
>
> * acknowledge that we would rather see Squid leaking than increasing the
> probability of a Squid crash after reconfigure.
>
> Both approaches are reasonable. The proposed patch does the former. I do
> not want to commit that patch because there is only one vote and it is "-0".
>
> What bothers me here is that the justification for the -0 vote was
> (AFAICT) a promise of a better patch, but it now appears that the better
> patch is not coming soon, has problems of its own, and may not even
> remove the leak. I cannot really combat those obstacles, and it is
> difficult for me to explain/justify them to others. May I re-interpret
> your -0 vote justification as "the proposed fix is too risky"?
>
>
> Thank you,
>
> Alex.
>