[PATCH] Avoid reconfiguration leak of objects tied to ports

[Alex Rousskov]

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.

Avoid reconfiguration leak of [SSL] objects tied to http_port and https_port.

=== modified file 'src/cache_cf.cc'
--- src/cache_cf.cc	2013-12-02 17:37:56 +0000
+++ src/cache_cf.cc	2014-02-11 19:35:19 +0000
@@ -3950,41 +3950,41 @@ dump_generic_port(StoreEntry * e, const 
 #endif
 }
 
 static void
 dump_PortCfg(StoreEntry * e, const char *n, const AnyP::PortCfg * s)
 {
     while (s) {
         dump_generic_port(e, n, s);
         storeAppendPrintf(e, "\n");
         s = s->next;
     }
 }
 
 static void
 free_PortCfg(AnyP::PortCfg ** head)
 {
     AnyP::PortCfg *s;
 
     while ((s = *head) != NULL) {
         *head = s->next;
-        cbdataReferenceDone(s);
+        delete s;
     }
 }
 
 void
 configFreeMemory(void)
 {
     free_all();
 #if USE_SSL
     SSL_CTX_free(Config.ssl_client.sslContext);
 #endif
 }
 
 void
 requirePathnameExists(const char *name, const char *path)
 {
 
     struct stat sb;
     char pathbuf[BUFSIZ];
     assert(path != NULL);