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 a78c9790bc0f94d67dbb793e1c6f439e37502fe7..be67b92554a55c8f7ad4545cee3151a5a2e69d62 100644 (file)
--- 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 3c5c56110c33bd71c137c3d0e5d2961b6993bb0e..a3aadb1d01d3db33c9123dde8661bb656f0d92a7 100644 (file)
--- 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 ee45680a51c03d0453567aea724cfa4866b088df..fdb389a0869e6f7f2f1715249148591101823703 100644 (file)
--- 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 b7739f33355338917768d2b10a8bf2c58fac944b..5b1d6f976c978e184f856723f5b4e15c49ccf5df 100644 (file)
--- 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 680d756a8ceb238438ef16bdb85b764fd23db5a2..c778338ece8e93638eb0c22dd273f004932a16e6 100644 (file)
--- 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