From 3a851845a5d5c7bc5046fd237848d1e6c5e73c13 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 31 Aug 2022 17:15:31 +0000
Subject: [PATCH] Remove unused/disabled/broken LEAK_CHECK_MODE code (#1130)

LEAK_CHECK_MODE had not worked since before 2006 commit afec404. The
disabled code does not compile since 2012 commit b65ce00 and would crash
current Squids during shutdown. We should not fix it because the
underlying "delete essentialService" idea is deeply flawed.

Writing correct code that uses essential modules/services that may
"suddenly" disappear or crash is nearly impossible. The trickle of
assertions due to missing Store::Root() is a case in point. These risks
and efforts are also unnecessary: We can and should build APIs that
provide essential services without disappearing or crashing. Keeping
heap-allocated service objects during shutdown helps with that.

Valgrind and other modern leak detection tools are capable of
distinguishing still-reachable at-exit memory from runtime memory leaks.
We do not need to delete all still-reachable at-exit objects to enable
that leak detection functionality.

The OS will reclaim allocated heap memory anyway.

N.B. Despite the legacy code implications, we should distinguish
low-level new/delete (a.k.a. fooInit() and fooFree()) memory management
code (which is an internal service matter that essential service users
should not be exposed to!) with configure-reconfigure-reconfigure-sync
events. There is value in notifying services about configuration
(changes) and various shutdown stages, of course. We already have the
RunnersRegistry API for handling such notifications.

Even without explicit "delete essentialService" calls, we may still have
problems during C++ post-exit() cleanup where destructors of various
globals are called "concurrently", with few order guarantees. We should
avoid non-heap-allocated globals with "complicated" destructors, but
their elimination is out of scope here.
---
 include/squid.h             |  7 -------
 src/Store.h                 |  3 ---
 src/client_db.cc            |  8 --------
 src/client_db.h             |  1 -
 src/event.cc                |  6 ------
 src/event.h                 |  1 -
 src/fqdncache.cc            | 18 ------------------
 src/fqdncache.h             |  1 -
 src/icmp/net_db.cc          | 32 --------------------------------
 src/icmp/net_db.h           |  1 -
 src/ipcache.cc              |  9 ---------
 src/ipcache.h               |  1 -
 src/main.cc                 | 15 ---------------
 src/stat.cc                 | 11 -----------
 src/stat.h                  |  1 -
 src/store.cc                | 10 ----------
 src/tests/stub_client_db.cc |  1 -
 src/tests/stub_event.cc     |  1 -
 src/tests/stub_fqdncache.cc |  1 -
 src/tests/stub_ipcache.cc   |  1 -
 src/tests/stub_libicmp.cc   |  1 -
 21 files changed, 130 deletions(-)

diff --git a/include/squid.h b/include/squid.h
index 6d9a66c05d..74652aede9 100644
--- a/include/squid.h
+++ b/include/squid.h
@@ -55,13 +55,6 @@
 #define SQUID_UDP_SO_RCVBUF SQUID_DETECT_UDP_SO_RCVBUF
 #endif
 
-/*
- * Determine if this is a leak check build or standard
- */
-#if PURIFY || WITH_VALGRIND
-#define LEAK_CHECK_MODE 1
-#endif
-
 /* temp hack: needs to be pre-defined for now. */
 #define SQUID_MAXPATHLEN 256
 
diff --git a/src/Store.h b/src/Store.h
index a8e686af0b..62894acffa 100644
--- a/src/Store.h
+++ b/src/Store.h
@@ -427,9 +427,6 @@ void storeInit(void);
 /// \ingroup StoreAPI
 void storeConfigure(void);
 
-/// \ingroup StoreAPI
-void storeFreeMemory(void);
-
 /// \ingroup StoreAPI
 int expiresMoreThan(time_t, time_t);
 
diff --git a/src/client_db.cc b/src/client_db.cc
index 994931605e..91bba5c594 100644
--- a/src/client_db.cc
+++ b/src/client_db.cc
@@ -346,14 +346,6 @@ ClientInfo::~ClientInfo()
     debugs(77, 9, "ClientInfo destructed, this=" << static_cast<void*>(this));
 }
 
-void
-clientdbFreeMemory(void)
-{
-    hashFreeItems(client_table, clientdbFreeItem);
-    hashFreeMemory(client_table);
-    client_table = nullptr;
-}
-
 static void
 clientdbScheduledGC(void *)
 {
diff --git a/src/client_db.h b/src/client_db.h
index 2fe603998c..66ff77d76c 100644
--- a/src/client_db.h
+++ b/src/client_db.h
@@ -30,7 +30,6 @@ class ClientInfo;
 void clientdbUpdate(const Ip::Address &, const LogTags &, AnyP::ProtocolType, size_t);
 int clientdbCutoffDenied(const Ip::Address &);
 void clientdbDump(StoreEntry *);
-void clientdbFreeMemory(void);
 int clientdbEstablished(const Ip::Address &, int);
 
 #if USE_DELAY_POOLS
diff --git a/src/event.cc b/src/event.cc
index cefdb22401..00c68d995d 100644
--- a/src/event.cc
+++ b/src/event.cc
@@ -141,12 +141,6 @@ eventDump(StoreEntry * sentry)
     EventScheduler::GetInstance()->dump(sentry);
 }
 
-void
-eventFreeMemory(void)
-{
-    EventScheduler::GetInstance()->clean();
-}
-
 int
 eventFind(EVH * func, void *arg)
 {
diff --git a/src/event.h b/src/event.h
index 2e81d0095a..31fc9ab7be 100644
--- a/src/event.h
+++ b/src/event.h
@@ -21,7 +21,6 @@ void eventAdd(const char *name, EVH * func, void *arg, double when, int, bool cb
 void eventAddIsh(const char *name, EVH * func, void *arg, double delta_ish, int);
 void eventDelete(EVH * func, void *arg);
 void eventInit(void);
-void eventFreeMemory(void);
 int eventFind(EVH *, void *);
 
 class ev_entry
diff --git a/src/fqdncache.cc b/src/fqdncache.cc
index a08a6d76fb..5cc354f77f 100644
--- a/src/fqdncache.cc
+++ b/src/fqdncache.cc
@@ -127,7 +127,6 @@ static fqdncache_entry *fqdncache_get(const char *);
 static int fqdncacheExpiredEntry(const fqdncache_entry *);
 static void fqdncacheLockEntry(fqdncache_entry * f);
 static void fqdncacheUnlockEntry(fqdncache_entry * f);
-static FREE fqdncacheFreeEntry;
 static void fqdncacheAddEntry(fqdncache_entry * f);
 
 /// \ingroup FQDNCacheInternal
@@ -601,14 +600,6 @@ fqdncacheUnlockEntry(fqdncache_entry * f)
         fqdncacheRelease(f);
 }
 
-/// \ingroup FQDNCacheInternal
-static void
-fqdncacheFreeEntry(void *data)
-{
-    fqdncache_entry *f = (fqdncache_entry *)data;
-    delete f;
-}
-
 fqdncache_entry::~fqdncache_entry()
 {
     for (int k = 0; k < (int)name_count; ++k)
@@ -618,15 +609,6 @@ fqdncache_entry::~fqdncache_entry()
     xfree(error_message);
 }
 
-/// \ingroup FQDNCacheAPI
-void
-fqdncacheFreeMemory(void)
-{
-    hashFreeItems(fqdn_table, fqdncacheFreeEntry);
-    hashFreeMemory(fqdn_table);
-    fqdn_table = nullptr;
-}
-
 /**
  \ingroup FQDNCacheAPI
  *
diff --git a/src/fqdncache.h b/src/fqdncache.h
index b5df1133b6..3b803b56ea 100644
--- a/src/fqdncache.h
+++ b/src/fqdncache.h
@@ -27,7 +27,6 @@ typedef void FQDNH(const char *, const Dns::LookupDetails &details, void *);
 
 void fqdncache_init(void);
 void fqdnStats(StoreEntry *);
-void fqdncacheFreeMemory(void);
 void fqdncache_restart(void);
 void fqdncache_purgelru(void *);
 void fqdncacheAddEntryFromHosts(char *addr, SBufList &hostnames);
diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc
index d7ed9ee906..cda165551e 100644
--- a/src/icmp/net_db.cc
+++ b/src/icmp/net_db.cc
@@ -106,8 +106,6 @@ static net_db_peer *netdbPeerByName(const netdbEntry * n, const char *);
 static net_db_peer *netdbPeerAdd(netdbEntry * n, CachePeer * e);
 static const char *netdbPeerName(const char *name);
 static IPH netdbSendPing;
-static FREE netdbFreeNameEntry;
-static FREE netdbFreeNetdbEntry;
 static STCB netdbExchangeHandleReply;
 
 /* We have to keep a local list of CachePeer names.  The Peers structure
@@ -666,21 +664,6 @@ netdbPeerName(const char *name)
     return wordlistAdd(&peer_names, name);
 }
 
-static void
-netdbFreeNetdbEntry(void *data)
-{
-    netdbEntry *n = (netdbEntry *)data;
-    safe_free(n->peers);
-    delete n;
-}
-
-static void
-netdbFreeNameEntry(void *data)
-{
-    net_db_name *x = (net_db_name *)data;
-    delete x;
-}
-
 static void
 netdbExchangeHandleReply(void *data, StoreIOBuffer receivedData)
 {
@@ -953,21 +936,6 @@ netdbHandlePingReply(const Ip::Address &from, int hops, int rtt)
 #endif
 }
 
-void
-netdbFreeMemory(void)
-{
-#if USE_ICMP
-    hashFreeItems(addr_table, netdbFreeNetdbEntry);
-    hashFreeMemory(addr_table);
-    addr_table = nullptr;
-    hashFreeItems(host_table, netdbFreeNameEntry);
-    hashFreeMemory(host_table);
-    host_table = nullptr;
-    wordlistDestroy(&peer_names);
-    peer_names = nullptr;
-#endif
-}
-
 void
 netdbDump(StoreEntry * sentry)
 {
diff --git a/src/icmp/net_db.h b/src/icmp/net_db.h
index 902dc7b97e..68a1b5af54 100644
--- a/src/icmp/net_db.h
+++ b/src/icmp/net_db.h
@@ -71,7 +71,6 @@ void netdbHandlePingReply(const Ip::Address &from, int hops, int rtt);
 void netdbPingSite(const char *hostname);
 void netdbDump(StoreEntry *);
 
-void netdbFreeMemory(void);
 int netdbHostHops(const char *host);
 int netdbHostRtt(const char *host);
 void netdbUpdatePeer(const AnyP::Uri &, CachePeer *, int rtt, int hops);
diff --git a/src/ipcache.cc b/src/ipcache.cc
index f03e579a42..e9b9811c70 100644
--- a/src/ipcache.cc
+++ b/src/ipcache.cc
@@ -1082,15 +1082,6 @@ ipcache_entry::~ipcache_entry()
     xfree(hash.key);
 }
 
-/// \ingroup IPCacheAPI
-void
-ipcacheFreeMemory(void)
-{
-    hashFreeItems(ip_table, ipcacheFreeEntry);
-    hashFreeMemory(ip_table);
-    ip_table = nullptr;
-}
-
 /**
  \ingroup IPCacheAPI
  *
diff --git a/src/ipcache.h b/src/ipcache.h
index 7b18e8fdcb..fe73c58ef0 100644
--- a/src/ipcache.h
+++ b/src/ipcache.h
@@ -226,7 +226,6 @@ void ipcacheInvalidateNegative(const char *);
 void ipcache_init(void);
 void ipcacheMarkBadAddr(const char *name, const Ip::Address &);
 void ipcacheMarkGoodAddr(const char *name, const Ip::Address &);
-void ipcacheFreeMemory(void);
 void ipcache_restart(void);
 int ipcacheAddEntryFromHosts(const char *name, const char *ipaddr);
 
diff --git a/src/main.cc b/src/main.cc
index 7258363044..64184e6547 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -2114,21 +2114,6 @@ SquidShutdown()
     Store::Root().sync();       /* Flush log close */
     StoreFileSystem::FreeAllFs();
     DiskIOModule::FreeAllModules();
-#if LEAK_CHECK_MODE && 0 /* doesn't work at the moment */
-
-    configFreeMemory();
-    storeFreeMemory();
-    /*stmemFreeMemory(); */
-    netdbFreeMemory();
-    ipcacheFreeMemory();
-    fqdncacheFreeMemory();
-    asnFreeMemory();
-    clientdbFreeMemory();
-    statFreeMemory();
-    eventFreeMemory();
-    mimeFreeMemory();
-    errorClean();
-#endif
     Store::FreeMemory();
 
     fdDumpOpen();
diff --git a/src/stat.cc b/src/stat.cc
index c17a50133f..e0b532253a 100644
--- a/src/stat.cc
+++ b/src/stat.cc
@@ -1569,17 +1569,6 @@ DumpCountersStats(Mgr::CountersActionData& stats, StoreEntry* sentry)
                       stats.hitValidationFailures);
 }
 
-void
-statFreeMemory(void)
-{
-    // TODO: replace with delete[]
-    for (int i = 0; i < N_COUNT_HIST; ++i)
-        CountHist[i] = StatCounters();
-
-    for (int i = 0; i < N_COUNT_HOUR_HIST; ++i)
-        CountHourHist[i] = StatCounters();
-}
-
 static void
 statPeerSelect(StoreEntry * sentry)
 {
diff --git a/src/stat.h b/src/stat.h
index 71c1fd4083..7397efd983 100644
--- a/src/stat.h
+++ b/src/stat.h
@@ -12,7 +12,6 @@
 #define SQUID_STAT_H_
 
 void statInit(void);
-void statFreeMemory(void);
 double median_svc_get(int, int);
 void pconnHistCount(int, int);
 int stat5minClientRequests(void);
diff --git a/src/store.cc b/src/store.cc
index ab732e7b7d..debb92655a 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -1307,16 +1307,6 @@ StoreEntry::negativeCache()
     }
 }
 
-void
-storeFreeMemory(void)
-{
-    Store::FreeMemory();
-#if USE_CACHE_DIGESTS
-    delete store_digest;
-#endif
-    store_digest = nullptr;
-}
-
 int
 expiresMoreThan(time_t expires, time_t when)
 {
diff --git a/src/tests/stub_client_db.cc b/src/tests/stub_client_db.cc
index a67df93c3b..ca6ddfd1c6 100644
--- a/src/tests/stub_client_db.cc
+++ b/src/tests/stub_client_db.cc
@@ -15,7 +15,6 @@
 void clientdbUpdate(const Ip::Address &, const LogTags &, AnyP::ProtocolType, size_t) STUB
 int clientdbCutoffDenied(const Ip::Address &) STUB_RETVAL(-1)
 void clientdbDump(StoreEntry *) STUB
-void clientdbFreeMemory(void) STUB
 int clientdbEstablished(const Ip::Address &, int) STUB_RETVAL(-1)
 #if USE_DELAY_POOLS
 void clientdbSetWriteLimiter(ClientInfo *, const int,const double,const double) STUB
diff --git a/src/tests/stub_event.cc b/src/tests/stub_event.cc
index 8946da350f..7e5d5449da 100644
--- a/src/tests/stub_event.cc
+++ b/src/tests/stub_event.cc
@@ -16,7 +16,6 @@ void eventAdd(const char *, EVH *, void *, double, int, bool) STUB_NOP
 void eventAddIsh(const char *, EVH *, void *, double, int) STUB
 void eventDelete(EVH *, void *) STUB
 void eventInit(void) STUB
-void eventFreeMemory(void) STUB
 int eventFind(EVH *, void *) STUB_RETVAL(-1)
 
 // ev_entry::ev_entry(char const * name, EVH * func, void *arg, double when, int weight, bool cbdata) STUB
diff --git a/src/tests/stub_fqdncache.cc b/src/tests/stub_fqdncache.cc
index 3bc612fbeb..8a1a12a779 100644
--- a/src/tests/stub_fqdncache.cc
+++ b/src/tests/stub_fqdncache.cc
@@ -16,7 +16,6 @@ bool Dns::ResolveClientAddressesAsap = false;
 
 void fqdncache_init(void) STUB
 void fqdnStats(StoreEntry *) STUB
-void fqdncacheFreeMemory(void) STUB
 void fqdncache_restart(void) STUB
 void fqdncache_purgelru(void *) STUB
 void fqdncacheAddEntryFromHosts(char *, SBufList &) STUB
diff --git a/src/tests/stub_ipcache.cc b/src/tests/stub_ipcache.cc
index 81a8e09295..b694f03595 100644
--- a/src/tests/stub_ipcache.cc
+++ b/src/tests/stub_ipcache.cc
@@ -20,7 +20,6 @@ void ipcacheInvalidateNegative(const char *) STUB
 void ipcache_init(void) STUB
 void ipcacheMarkBadAddr(const char *, const Ip::Address &) STUB
 void ipcacheMarkGoodAddr(const char *, const Ip::Address &) STUB
-void ipcacheFreeMemory(void) STUB
 void ipcache_restart(void) STUB
 int ipcacheAddEntryFromHosts(const char *, const char *) STUB_RETVAL(-1)
 
diff --git a/src/tests/stub_libicmp.cc b/src/tests/stub_libicmp.cc
index 658fe8af6f..fc4c06a541 100644
--- a/src/tests/stub_libicmp.cc
+++ b/src/tests/stub_libicmp.cc
@@ -27,7 +27,6 @@ void netdbInit(void) STUB
 void netdbHandlePingReply(const Ip::Address &, int, int) STUB
 void netdbPingSite(const char *) STUB
 void netdbDump(StoreEntry *) STUB
-void netdbFreeMemory(void) STUB
 int netdbHostHops(const char *) STUB_RETVAL(-1)
 int netdbHostRtt(const char *) STUB_RETVAL(-1)
 void netdbUpdatePeer(const AnyP::Uri &, CachePeer *, int, int) STUB
-- 
2.39.5