From: Craig Gowing <craiggowing@gmail.com>
Date: Thu, 3 Oct 2019 23:11:48 +0000 (+0000)
Subject: Bug 4989: Leaking StoreEntry objects on Cache Digest rebuilds (#487)
X-Git-Tag: SQUID_5_0_1~32
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=508590d942a905cfc3301df3d83fe417c402c5ee;p=thirdparty%2Fsquid.git

Bug 4989: Leaking StoreEntry objects on Cache Digest rebuilds (#487)

When writing a newly generated Cache Digest to cache, Squid relied on a
cache key collision to purge the old digest entry. Since 4310f8b, the
collision resolution method -- forcePublicKey() -- leaked an idle (i.e.
lock_count=0) digest entry. If Squid still had unlocked entries lying
around, then the problem could extend to clashes unrelated to Digests.

Until 4310f8b, StoreEntry::forcePublicKey() called setPrivateKey()
before releasing the old entry. That explicit call was wasteful in many
cases, but, unbeknownst to its removal authors, it allowed release() to
destroy an idle Cache Digest entry by effectively disabling the
ENTRY_SPECIAL hack in StoreEntry::locked().

This change removes the ENTRY_SPECIAL hack in StoreEntry::locked(),
addressing an old TODO. The two ENTRY_SPECIAL creators (icons and Cache
Digests) now lock their entries to prevent their unwanted destruction.

Also explicitly release the old Cache Digest entry (instead of relying
on the implicit key collision) to avoid the unchecked assumption that
the Cache Digest key never changes.
---

diff --git a/src/Store.h b/src/Store.h
index a78c9790bc..be67b92554 100644
--- a/src/Store.h
+++ b/src/Store.h
@@ -128,7 +128,7 @@ public:
     /// TODO: Rename and make private so only those two methods can call this.
     bool checkCachable();
     int checkNegativeHit() const;
-    int locked() const;
+    int locked() const { return lock_count; }
     int validToSend() const;
     bool memoryCachable(); ///< checkCachable() and can be cached in memory
 
diff --git a/src/mime.cc b/src/mime.cc
index 3c5c56110c..a3aadb1d01 100644
--- a/src/mime.cc
+++ b/src/mime.cc
@@ -436,7 +436,8 @@ MimeIcon::created(StoreEntry *newEntry)
     e->flush();
     e->complete();
     e->timestampsSet();
-    e->unlock("MimeIcon::created");
+    // MimeIcons are only loaded once, prevent accidental destruction
+    // e->unlock("MimeIcon::created");
     debugs(25, 3, "Loaded icon " << url_);
 }
 
diff --git a/src/store.cc b/src/store.cc
index ee45680a51..fdb389a086 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -1250,24 +1250,6 @@ storeLateRelease(void *)
     eventAdd("storeLateRelease", storeLateRelease, NULL, 0.0, 1);
 }
 
-/* return 1 if a store entry is locked */
-int
-StoreEntry::locked() const
-{
-    if (lock_count)
-        return 1;
-
-    /*
-     * SPECIAL, PUBLIC entries should be "locked";
-     * XXX: Their owner should lock them then instead of relying on this hack.
-     */
-    if (EBIT_TEST(flags, ENTRY_SPECIAL))
-        if (!EBIT_TEST(flags, KEY_PRIVATE))
-            return 1;
-
-    return 0;
-}
-
 bool
 StoreEntry::validLength() const
 {
diff --git a/src/store_digest.cc b/src/store_digest.cc
index b7739f3335..5b1d6f976c 100644
--- a/src/store_digest.cc
+++ b/src/store_digest.cc
@@ -47,6 +47,7 @@ public:
     StoreDigestCBlock cblock;
     int rebuild_lock = 0;                 ///< bucket number
     StoreEntry * rewrite_lock = nullptr;  ///< points to store entry with the digest
+    StoreEntry * publicEntry = nullptr;  ///< points to the previous store entry with the digest
     StoreSearchPointer theSearch;
     int rewrite_offset = 0;
     int rebuild_count = 0;
@@ -445,8 +446,15 @@ storeDigestRewriteResume(void)
     e = sd_state.rewrite_lock;
     sd_state.rewrite_offset = 0;
     EBIT_SET(e->flags, ENTRY_SPECIAL);
-    /* setting public key will purge old digest entry if any */
+    /* setting public key will mark the old digest entry for removal once unlocked */
     e->setPublicKey();
+    if (const auto oldEntry = sd_state.publicEntry) {
+        oldEntry->release(true);
+        sd_state.publicEntry = nullptr;
+        oldEntry->unlock("storeDigestRewriteResume");
+    }
+    assert(e->locked());
+    sd_state.publicEntry = e;
     /* fake reply */
     HttpReply *rep = new HttpReply;
     rep->setHeaders(Http::scOkay, "Cache Digest OK",
@@ -472,7 +480,6 @@ storeDigestRewriteFinish(StoreEntry * e)
            " (" << std::showpos << (int) (e->expires - squid_curtime) << ")");
     /* is this the write order? @?@ */
     e->mem_obj->unlinkRequest();
-    e->unlock("storeDigestRewriteFinish");
     sd_state.rewrite_lock = NULL;
     ++sd_state.rewrite_count;
     eventAdd("storeDigestRewriteStart", storeDigestRewriteStart, NULL, (double)
diff --git a/src/tests/stub_store.cc b/src/tests/stub_store.cc
index 680d756a8c..c778338ece 100644
--- a/src/tests/stub_store.cc
+++ b/src/tests/stub_store.cc
@@ -50,7 +50,6 @@ void StoreEntry::swapOutFileClose(int how) STUB
 const char *StoreEntry::url() const STUB_RETVAL(NULL)
 bool StoreEntry::checkCachable() STUB_RETVAL(false)
 int StoreEntry::checkNegativeHit() const STUB_RETVAL(0)
-int StoreEntry::locked() const STUB_RETVAL(0)
 int StoreEntry::validToSend() const STUB_RETVAL(0)
 bool StoreEntry::memoryCachable() STUB_RETVAL(false)
 void StoreEntry::createMemObject() STUB