Bug 5134: assertion failed: Transients.cc:221: "old == e" (#958)

Make sure the StoreMap anchor we open for reading has our key (and not
just happens to be at our hash position). Prior to this change, two
openForReadingAt() calls were missing a sameKey() post-call check due to
a buggy backport (v5 commit ec50061; mis-attributed to me).

Now the sameKey() check is integrated into the openForReadingAt() method
itself, as was already done in master/v6 since commit b2aca62.

diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index e1590d7792308fc2c67ff02f5745c4b24c520290..fb304d655f14e4e55a372784d2080a26f436f33f 100644 (file)
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -102,7 +102,7 @@ Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &filen
 
     // start with reading so that we do not overwrite an existing unlocked entry
     auto idx = fileNoByKey(key);
-    if (const auto anchor = openForReadingAt(idx)) {
+    if (const auto anchor = openForReadingAt(idx, key)) {
         fileno = idx;
         return anchor;
     }
@@ -122,7 +122,7 @@ Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &filen
     // we lost the above race; see if the winner-created entry is now readable
     // TODO: Do some useful housekeeping work here to give the winner more time.
     idx = fileNoByKey(key);
-    if (const auto anchor = openForReadingAt(idx)) {
+    if (const auto anchor = openForReadingAt(idx, key)) {
         fileno = idx;
         return anchor;
     }
@@ -437,19 +437,15 @@ Ipc::StoreMap::openForReading(const cache_key *const key, sfileno &fileno)
     debugs(54, 5, "opening entry with key " << storeKeyText(key)
            << " for reading " << path);
     const int idx = fileNoByKey(key);
-    if (const Anchor *slot = openForReadingAt(idx)) {
-        if (slot->sameKey(key)) {
-            fileno = idx;
-            return slot; // locked for reading
-        }
-        slot->lock.unlockShared();
-        debugs(54, 7, "closed wrong-key entry " << idx << " for reading " << path);
+    if (const auto anchor = openForReadingAt(idx, key)) {
+        fileno = idx;
+        return anchor; // locked for reading
     }
     return NULL;
 }
 
 const Ipc::StoreMap::Anchor *
-Ipc::StoreMap::openForReadingAt(const sfileno fileno)
+Ipc::StoreMap::openForReadingAt(const sfileno fileno, const cache_key *const key)
 {
     debugs(54, 5, "opening entry " << fileno << " for reading " << path);
     Anchor &s = anchorAt(fileno);
@@ -474,6 +470,13 @@ Ipc::StoreMap::openForReadingAt(const sfileno fileno)
         return NULL;
     }
 
+    if (!s.sameKey(key)) {
+        s.lock.unlockShared();
+        debugs(54, 5, "cannot open wrong-key entry " << fileno <<
+               " for reading " << path);
+        return nullptr;
+    }
+
     debugs(54, 5, "opened entry " << fileno << " for reading " << path);
     return &s;
 }
@@ -524,13 +527,7 @@ Ipc::StoreMap::openForUpdating(Update &update, const sfileno fileNoHint)
 
     // Unreadable entries cannot (e.g., empty and otherwise problematic entries)
     // or should not (e.g., entries still forming their metadata) be updated.
-    if (const Anchor *anchor = openForReadingAt(update.stale.fileNo)) {
-        if (!anchor->sameKey(key)) {
-            closeForReading(update.stale.fileNo);
-            debugs(54, 5, "cannot open wrong-key entry " << update.stale.fileNo << " for updating " << path);
-            return false;
-        }
-    } else {
+    if (!openForReadingAt(update.stale.fileNo, key)) {
         debugs(54, 5, "cannot open unreadable entry " << update.stale.fileNo << " for updating " << path);
         return false;
     }

diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h
index 739c8da81e467e593bb08140918e953dfd3d91cb..e2bc37e3d9201d5b820f1484654547d0dd3edd2d 100644 (file)
--- a/src/ipc/StoreMap.h
+++ b/src/ipc/StoreMap.h
@@ -288,7 +288,7 @@ public:
     /// opens entry (identified by key) for reading, increments read level
     const Anchor *openForReading(const cache_key *const key, sfileno &fileno);
     /// opens entry (identified by sfileno) for reading, increments read level
-    const Anchor *openForReadingAt(const sfileno fileno);
+    const Anchor *openForReadingAt(const sfileno, const cache_key *const);
     /// closes open entry after reading, decrements read level
     void closeForReading(const sfileno fileno);
     /// same as closeForReading() but also frees the entry if it is unlocked