Bug 4088: memory leak in external_acl_type helper with cache=0 or ttl=0

ExternalACLEntry / external_acl_entry objects have been abusing the
CBDATA API for reference counting and since 3.4 this has resulted in
hidden memory leaks as object accounting shows all locks released but
the memory is not freed by any 'owner'.

* convert to using RefCount<> API.

* move ExternalACLEntry pre-define to acl/forward.h

* add ExternalACLEntryPointer in acl/forward.h

* convert LookupDone() method to using explicit typed pointer

* convert from CBDATA_CLASS to MEMPROXY_CLASS memory management.

* convert almost all raw ExternalACLEntry* to Pointer
 - remaining usage is in the cache hash pointers. Use an explicit 'cachd'
  lock/unlock until this hash is updated to std:: structure types.

diff --git a/src/ExternalACL.h b/src/ExternalACL.h
index faf6630ac5b52591303c734aa8b8e5d72bb8f848..5727fbedd1a4894fd3e335c03d26e5e2c9ed9edc 100644 (file)
--- a/src/ExternalACL.h
+++ b/src/ExternalACL.h
@@ -10,6 +10,8 @@
 #define SQUID_EXTERNALACL_H
 
 #include "acl/Checklist.h"
+#include "base/RefCount.h"
+
 class external_acl;
 class StoreEntry;
 
@@ -29,7 +31,7 @@ public:
 
 private:
     static ExternalACLLookup instance_;
-    static void LookupDone(void *data, void *result);
+    static void LookupDone(void *data, const ExternalACLEntryPointer &result);
 };
 
 #include "acl/Acl.h"
@@ -73,7 +75,7 @@ MEMPROXY_CLASS_INLINE(ACLExternal);
 void parse_externalAclHelper(external_acl **);
 void dump_externalAclHelper(StoreEntry * sentry, const char *name, const external_acl *);
 void free_externalAclHelper(external_acl **);
-typedef void EAH(void *data, void *result);
+typedef void EAH(void *data, const ExternalACLEntryPointer &result);
 void externalAclLookup(ACLChecklist * ch, void *acl_data, EAH * handler, void *data);
 void externalAclInit(void);
 void externalAclShutdown(void);

diff --git a/src/ExternalACLEntry.cc b/src/ExternalACLEntry.cc
index 8038636007208e3bd2caa4d2052b4e08e023906b..da8e4af9e37df94f630ea697579f53459910b468 100644 (file)
--- a/src/ExternalACLEntry.cc
+++ b/src/ExternalACLEntry.cc
@@ -16,8 +16,6 @@
  * external_acl cache
  */
 
-CBDATA_CLASS_INIT(ExternalACLEntry);
-
 ExternalACLEntry::ExternalACLEntry() :
         notes()
 {

diff --git a/src/ExternalACLEntry.h b/src/ExternalACLEntry.h
index 35bdef2645eb2215213df0643fa993a3e627871c..7b94cd8cebc0feace0f33fc75ac9cdc99f8fbefb 100644 (file)
--- a/src/ExternalACLEntry.h
+++ b/src/ExternalACLEntry.h
@@ -12,7 +12,7 @@
 #define SQUID_EXTERNALACLENTRY_H
 
 #include "acl/Acl.h"
-#include "cbdata.h"
+#include "acl/forward.h"
 #include "hash.h"
 #include "Notes.h"
 #include "SquidString.h"
@@ -51,9 +51,8 @@ public:
  * Used opaqueue in the interface
  */
 
-class ExternalACLEntry: public hash_link
+class ExternalACLEntry: public hash_link, public RefCountable
 {
-
 public:
     ExternalACLEntry();
     ~ExternalACLEntry();
@@ -75,10 +74,9 @@ public:
     String log;
     external_acl *def;
 
-private:
-    CBDATA_CLASS2(ExternalACLEntry);
+    MEMPROXY_CLASS(ExternalACLEntry);
 };
 
-typedef class ExternalACLEntry external_acl_entry;
+MEMPROXY_CLASS_INLINE(ExternalACLEntry);
 
 #endif

diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index 1cac2dd704bec55101e63e7ec0fc3137e1c681b8..b90577d165058455e6260422088856fbea759d44 100644 (file)
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -11,6 +11,7 @@
 #include "client_side.h"
 #include "comm/Connection.h"
 #include "comm/forward.h"
+#include "ExternalACLEntry.h"
 #include "HttpReply.h"
 #include "HttpRequest.h"
 #include "SquidConfig.h"
@@ -35,7 +36,6 @@ ACLFilledChecklist::ACLFilledChecklist() :
 #if USE_OPENSSL
         sslErrors(NULL),
 #endif
-        extacl_entry (NULL),
         conn_(NULL),
         fd_(-1),
         destinationDomainChecked_(false),
@@ -53,9 +53,6 @@ ACLFilledChecklist::~ACLFilledChecklist()
 
     safe_free(dst_rdns); // created by xstrdup().
 
-    if (extacl_entry)
-        cbdataReferenceDone(extacl_entry);
-
     HTTPMSGUNLOCK(request);
 
     HTTPMSGUNLOCK(reply);
@@ -151,7 +148,6 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re
 #if USE_OPENSSL
         sslErrors(NULL),
 #endif
-        extacl_entry (NULL),
         conn_(NULL),
         fd_(-1),
         destinationDomainChecked_(false),

diff --git a/src/acl/FilledChecklist.h b/src/acl/FilledChecklist.h
index 146148b47c6bbf794716d70ae50c333c66d9c78e..3b40a12350bcd3257396bd8926183989cd9b2cbd 100644 (file)
--- a/src/acl/FilledChecklist.h
+++ b/src/acl/FilledChecklist.h
@@ -12,6 +12,7 @@
 #include "AccessLogEntry.h"
 #include "acl/Checklist.h"
 #include "acl/forward.h"
+#include "base/CbcPointer.h"
 #include "ip/Address.h"
 #if USE_AUTH
 #include "auth/UserRequest.h"
@@ -22,7 +23,6 @@
 
 class CachePeer;
 class ConnStateData;
-class ExternalACLEntry;
 class HttpRequest;
 class HttpReply;
 
@@ -86,7 +86,7 @@ public:
 
     AccessLogEntry::Pointer al; ///< info for the future access.log entry
 
-    ExternalACLEntry *extacl_entry;
+    ExternalACLEntryPointer extacl_entry;
 
 private:
     ConnStateData * conn_;          /**< hack for ident and NTLM */

diff --git a/src/acl/forward.h b/src/acl/forward.h
index aea6c45beb51e38ba0711e5c538e935827cd2748..67d304cdd1140dcb65f0f967f46b8843938c40f7 100644 (file)
--- a/src/acl/forward.h
+++ b/src/acl/forward.h
@@ -9,6 +9,8 @@
 #ifndef SQUID_ACL_FORWARD_H
 #define SQUID_ACL_FORWARD_H
 
+#include "base/RefCount.h"
+
 class ACL;
 class ACLChecklist;
 class ACLFilledChecklist;
@@ -39,4 +41,7 @@ typedef void ACLCB(allow_t, void *);
 #define acl_access Acl::Tree
 #define ACLList Acl::Tree
 
+class ExternalACLEntry;
+typedef RefCount<ExternalACLEntry> ExternalACLEntryPointer;
+
 #endif /* SQUID_ACL_FORWARD_H */

diff --git a/src/external_acl.cc b/src/external_acl.cc
index cf9842418b65ee0eec7081a8691a984312b3fca5..35bc9ab92a5f9a9bfa85cfd23dc775af3039af35 100644 (file)
--- a/src/external_acl.cc
+++ b/src/external_acl.cc
@@ -56,11 +56,11 @@
 #endif
 
 static char *makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data);
-static void external_acl_cache_delete(external_acl * def, external_acl_entry * entry);
-static int external_acl_entry_expired(external_acl * def, external_acl_entry * entry);
-static int external_acl_grace_expired(external_acl * def, external_acl_entry * entry);
-static void external_acl_cache_touch(external_acl * def, external_acl_entry * entry);
-static external_acl_entry *external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData const &data);
+static void external_acl_cache_delete(external_acl * def, const ExternalACLEntryPointer &entry);
+static int external_acl_entry_expired(external_acl * def, const ExternalACLEntryPointer &entry);
+static int external_acl_grace_expired(external_acl * def, const ExternalACLEntryPointer &entry);
+static void external_acl_cache_touch(external_acl * def, const ExternalACLEntryPointer &entry);
+static ExternalACLEntryPointer external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData const &data);
 
 /******************************************************************
  * external_acl directive
@@ -94,7 +94,7 @@ class external_acl
 public:
     external_acl *next;
 
-    void add(ExternalACLEntry *);
+    void add(const ExternalACLEntryPointer &);
 
     void trimCache();
 
@@ -163,8 +163,10 @@ free_external_acl(void *data)
         p->theHelper = NULL;
     }
 
-    while (p->lru_list.tail)
-        external_acl_cache_delete(p, static_cast<external_acl_entry *>(p->lru_list.tail->data));
+    while (p->lru_list.tail) {
+        ExternalACLEntryPointer e(static_cast<ExternalACLEntry *>(p->lru_list.tail->data));
+        external_acl_cache_delete(p, e);
+    }
     if (p->cache)
         hashFreeMemory(p->cache);
 }
@@ -580,21 +582,26 @@ find_externalAclHelper(const char *name)
 }
 
 void
-external_acl::add(ExternalACLEntry *anEntry)
+external_acl::add(const ExternalACLEntryPointer &anEntry)
 {
     trimCache();
+    assert(anEntry != NULL);
     assert (anEntry->def == NULL);
     anEntry->def = this;
-    hash_join(cache, anEntry);
-    dlinkAdd(anEntry, &anEntry->lru, &lru_list);
+    ExternalACLEntry *e = const_cast<ExternalACLEntry *>(anEntry.getRaw()); // XXX: make hash a std::map of Pointer.
+    hash_join(cache, e);
+    dlinkAdd(e, &e->lru, &lru_list);
+    e->lock(); //cbdataReference(e); // lock it on behalf of the hash
     ++cache_entries;
 }
 
 void
 external_acl::trimCache()
 {
-    if (cache_size && cache_entries >= cache_size)
-        external_acl_cache_delete(this, static_cast<external_acl_entry *>(lru_list.tail->data));
+    if (cache_size && cache_entries >= cache_size) {
+        ExternalACLEntryPointer e(static_cast<ExternalACLEntry *>(lru_list.tail->data));
+        external_acl_cache_delete(this, e);
+    }
 }
 
 /******************************************************************
@@ -681,7 +688,7 @@ ACLExternal::~ACLExternal()
 }
 
 static void
-copyResultsFromEntry(HttpRequest *req, external_acl_entry *entry)
+copyResultsFromEntry(HttpRequest *req, const ExternalACLEntryPointer &entry)
 {
     if (req) {
 #if USE_AUTH
@@ -706,32 +713,30 @@ static allow_t
 aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
 {
     debugs(82, 9, HERE << "acl=\"" << acl->def->name << "\"");
-    external_acl_entry *entry = ch->extacl_entry;
+    ExternalACLEntryPointer entry = ch->extacl_entry;
 
     external_acl_message = "MISSING REQUIRED INFORMATION";
 
-    if (entry) {
-        if (cbdataReferenceValid(entry) && entry->def == acl->def) {
+    if (entry != NULL) {
+        if (entry->def == acl->def) {
             /* Ours, use it.. if the key matches */
             const char *key = makeExternalAclKey(ch, acl);
             if (!key)
                 return ACCESS_DUNNO; // insufficent data to continue
             if (strcmp(key, (char*)entry->key) != 0) {
-                debugs(82, 9, HERE << "entry key='" << (char *)entry->key << "', our key='" << key << "' dont match. Discarded.");
+                debugs(82, 9, "entry key='" << (char *)entry->key << "', our key='" << key << "' dont match. Discarded.");
                 // too bad. need a new lookup.
-                cbdataReferenceDone(ch->extacl_entry);
-                entry = NULL;
+                entry = ch->extacl_entry = NULL;
             }
         } else {
-            /* Not valid, or not ours.. get rid of it */
-            debugs(82, 9, HERE << "entry " << entry << " not valid or not ours. Discarded.");
-            if (entry) {
-                debugs(82, 9, HERE << "entry def=" << entry->def << ", our def=" << acl->def);
+            /* Not ours.. get rid of it */
+            debugs(82, 9, "entry " << entry << " not valid or not ours. Discarded.");
+            if (entry != NULL) {
+                debugs(82, 9, "entry def=" << entry->def << ", our def=" << acl->def);
                 const char *key = makeExternalAclKey(ch, acl); // may be nil
-                debugs(82, 9, HERE << "entry key='" << (char *)entry->key << "', our key='" << key << "'");
+                debugs(82, 9, "entry key='" << (char *)entry->key << "', our key='" << key << "'");
             }
-            cbdataReferenceDone(ch->extacl_entry);
-            entry = NULL;
+            entry = ch->extacl_entry = NULL;
         }
     }
 
@@ -756,13 +761,13 @@ aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
             return ACCESS_DUNNO;
         }
 
-        entry = static_cast<external_acl_entry *>(hash_lookup(acl->def->cache, key));
+        entry = static_cast<ExternalACLEntry *>(hash_lookup(acl->def->cache, key));
 
-        external_acl_entry *staleEntry = entry;
-        if (entry && external_acl_entry_expired(acl->def, entry))
+        const ExternalACLEntryPointer staleEntry = entry;
+        if (entry != NULL && external_acl_entry_expired(acl->def, entry))
             entry = NULL;
 
-        if (entry && external_acl_grace_expired(acl->def, entry)) {
+        if (entry != NULL && external_acl_grace_expired(acl->def, entry)) {
             // refresh in the background
             ExternalACLLookup::Start(ch, acl, true);
             debugs(82, 4, HERE << "no need to wait for the refresh of '" <<
@@ -771,8 +776,6 @@ aclMatchExternal(external_acl_data *acl, ACLFilledChecklist *ch)
 
         if (!entry) {
             debugs(82, 2, HERE << acl->def->name << "(\"" << key << "\") = lookup needed");
-            debugs(82, 2, HERE << "\"" << key << "\": entry=@" <<
-                   entry << ", age=" << (entry ? (long int) squid_curtime - entry->date : 0));
 
             if (acl->def->theHelper->stats.queue_size < (int)acl->def->theHelper->childs.n_active) {
                 debugs(82, 2, HERE << "\"" << key << "\": queueing a call.");
@@ -858,14 +861,15 @@ ACLExternal::dump() const
  */
 
 static void
-external_acl_cache_touch(external_acl * def, external_acl_entry * entry)
+external_acl_cache_touch(external_acl * def, const ExternalACLEntryPointer &entry)
 {
     // this must not be done when nothing is being cached.
     if (def->cache_size <= 0 || (def->ttl <= 0 && entry->result == 1) || (def->negative_ttl <= 0 && entry->result != 1))
         return;
 
     dlinkDelete(&entry->lru, &def->lru_list);
-    dlinkAdd(entry, &entry->lru, &def->lru_list);
+    ExternalACLEntry *e = const_cast<ExternalACLEntry *>(entry.getRaw()); // XXX: make hash a std::map of Pointer.
+    dlinkAdd(e, &entry->lru, &def->lru_list);
 }
 
 static char *
@@ -1166,7 +1170,7 @@ makeExternalAclKey(ACLFilledChecklist * ch, external_acl_data * acl_data)
 }
 
 static int
-external_acl_entry_expired(external_acl * def, external_acl_entry * entry)
+external_acl_entry_expired(external_acl * def, const ExternalACLEntryPointer &entry)
 {
     if (def->cache_size <= 0)
         return 1;
@@ -1178,7 +1182,7 @@ external_acl_entry_expired(external_acl * def, external_acl_entry * entry)
 }
 
 static int
-external_acl_grace_expired(external_acl * def, external_acl_entry * entry)
+external_acl_grace_expired(external_acl * def, const ExternalACLEntryPointer &entry)
 {
     if (def->cache_size <= 0)
         return 1;
@@ -1193,10 +1197,10 @@ external_acl_grace_expired(external_acl * def, external_acl_entry * entry)
         return 0;
 }
 
-static external_acl_entry *
+static ExternalACLEntryPointer
 external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData const & data)
 {
-    ExternalACLEntry *entry;
+    ExternalACLEntryPointer entry;
 
     // do not bother caching this result if TTL is going to expire it immediately
     if (def->cache_size <= 0 || (def->ttl <= 0 && data.result == 1) || (def->negative_ttl <= 0 && data.result != 1)) {
@@ -1211,11 +1215,10 @@ external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData
     entry = static_cast<ExternalACLEntry *>(hash_lookup(def->cache, key));
     debugs(82, 2, "external_acl_cache_add: Adding '" << key << "' = " << data.result);
 
-    if (entry) {
-        debugs(82, 3, "ExternalACLEntry::update: updating existing entry");
+    if (entry != NULL) {
+        debugs(82, 3, "updating existing entry");
         entry->update(data);
         external_acl_cache_touch(def, entry);
-
         return entry;
     }
 
@@ -1229,13 +1232,15 @@ external_acl_cache_add(external_acl * def, const char *key, ExternalACLEntryData
 }
 
 static void
-external_acl_cache_delete(external_acl * def, external_acl_entry * entry)
+external_acl_cache_delete(external_acl * def, const ExternalACLEntryPointer &entry)
 {
+    assert(entry != NULL);
     assert(def->cache_size > 0 && entry->def == def);
-    hash_remove_link(def->cache, entry);
-    dlinkDelete(&entry->lru, &def->lru_list);
+    ExternalACLEntry *e = const_cast<ExternalACLEntry *>(entry.getRaw()); // XXX: make hash a std::map of Pointer.
+    hash_remove_link(def->cache, e);
+    dlinkDelete(&e->lru, &def->lru_list);
+    e->unlock(); // unlock on behalf of the hash
     def->cache_entries -= 1;
-    delete entry;
 }
 
 /******************************************************************
@@ -1292,7 +1297,6 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
     externalAclState *next;
     ExternalACLEntryData entryData;
     entryData.result = ACCESS_DENIED;
-    external_acl_entry *entry = NULL;
 
     debugs(82, 2, HERE << "reply=" << reply);
 
@@ -1328,14 +1332,15 @@ externalAclHandleReply(void *data, const Helper::Reply &reply)
 
     dlinkDelete(&state->list, &state->def->queue);
 
+    ExternalACLEntryPointer entry;
     if (cbdataReferenceValid(state->def)) {
         // only cache OK and ERR results.
         if (reply.result == Helper::Okay || reply.result == Helper::Error)
             entry = external_acl_cache_add(state->def, state->key, entryData);
         else {
-            external_acl_entry *oldentry = (external_acl_entry *)hash_lookup(state->def->cache, state->key);
+            const ExternalACLEntryPointer oldentry = static_cast<ExternalACLEntry *>(hash_lookup(state->def->cache, state->key));
 
-            if (oldentry)
+            if (oldentry != NULL)
                 external_acl_cache_delete(state->def, oldentry);
         }
     }
@@ -1523,13 +1528,13 @@ ExternalACLLookup::checkForAsync(ACLChecklist *checklist)const
 
 /// Called when an async lookup returns
 void
-ExternalACLLookup::LookupDone(void *data, void *result)
+ExternalACLLookup::LookupDone(void *data, const ExternalACLEntryPointer &result)
 {
     ACLFilledChecklist *checklist = Filled(static_cast<ACLChecklist*>(data));
-    checklist->extacl_entry = cbdataReference((external_acl_entry *)result);
+    checklist->extacl_entry = result;
 
     // attach the helper kv-pair to the transaction
-    if (checklist->extacl_entry) {
+    if (checklist->extacl_entry != NULL) {
         if (HttpRequest * req = checklist->request) {
             // XXX: we have no access to the transaction / AccessLogEntry so cant SyncNotes().
             // workaround by using anything already set in HttpRequest