Re: [PATCH] Avoid reconfiguration leak of objects tied to ports

On 2014-02-13 08:17, Alex Rousskov wrote:
> Hello,
>
> The attached patch avoids reconfiguration leak of [SSL] objects
> tied
> to http_port and https_port.
>
> This one-line change is rather controversial, and I am not sure it
> should be accepted. The change fixes a true bug that causes a true
> memory leak, but the rest of Squid code may not be ready for this fix.
>
> Specifically, after this change, SSL-related objects stored in PortCfg
> objects will be properly destroyed on reconfigure and the
> cbdata-protected PortCfg object pointers will be invalidated. This is
> good, but lots of Squid code dereferences those pointers without
> checking for their validity. Leaks aside, that worked OK in most cases
> because:
>
> A) Old POD members of PortCfg object remain in memory until the last
> port user calls cbdataReferenceDone(), triggering freeing of the port
> object itself (in C terms).
>
> B) Newer non-POD members that are supposed to be freed/deleted by the
> PortCfg destructor were never deleted because the destructor was never
> called (there was no "delete port" code, only cbdataReferenceDone
> calls). Any code referring to those members would work for (A) reasons.
>
> After this change, the (A) case would still work as before, but (B) may
> break because the (B) members will be immediately deleted upon
> reconfigure. As of now, I do not know of any specific cases where (B)
> members are dereferenced after reconfigure, but I have not traced all
> non-POD member usage (there are too many of them to do it quickly).
>
> Long-term, PortCfg should be refcounted instead of cbdata-protected. It
> is essentially used as a refcounted object today, abusing cbdata API.
> This long-term fix will solve this problem completely, but that is a
> relatively big change, affecting a lot of code (albeit in trivial
> way?).
> Can anybody volunteer to implement this?
>
> Short-term, we have to choose between:
>
> 1) Leaky reconfigure when ports have SSL options (current code).
> 2) Crash dangers when ports have SSL options (patched code).
>
>
> Cheers,
>
> Alex.

The SSL objects may also separately be involved with any of the
ssl-bumping cert caches and paths.

Users who reconfigure frequently may consider the risk less danger than
the crashes, so having this attached to a bug report would be good. But
I don't think it should be applied at this time.

Fully agree on the ref-counting. That needs to start with making the SSL
objects used by the port object link to it with smart pointers so they
are also refcounted separately, otherwise the same risk is added
regardless of the PortCfg protection.

Amos
Received on Wed Feb 12 2014 - 22:17:59 MST