[PATCH] Avoid reconfiguration leak of objects tied to ports

From: Alex Rousskov <rousskov_at_measurement-factory.com>
Date: Wed, 12 Feb 2014 12:17:03 -0700

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.

Received on Wed Feb 12 2014 - 19:17:08 MST

This archive was generated by hypermail 2.2.0 : Thu Feb 13 2014 - 12:00:12 MST