From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Wed, 17 May 2023 19:33:51 +0000 (+0000)
Subject: Do not erase aborted StoreMap entries that are still being read (#1344)
X-Git-Tag: SQUID_6_0_3~7
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a776384202f76a9d959dcf91c7825911ec789323;p=thirdparty%2Fsquid.git

Do not erase aborted StoreMap entries that are still being read (#1344)

The old `!s.lock.readers` precondition for calling freeChain() in
StoreMap::abortWriting() was not broad enough: It covered "old" readers
that have incremented ReadWriteLock::readers already while missing those
lockShared() callers that have fully qualified for becoming a reader but
have not incremented the `readers` counter yet. A correct test is now
implemented by ReadWriteLock::stopAppendingAndRestoreExclusive().

This code was broken since appending mode was introduced in 2013 commit
ce49546. Evidently, the mishandled race condition is short-lived enough
and abortWriting() calls are rare enough, that there are no (known)
relevant bug reports. This bug was accidentally discovered while reading
the code to explain another SMP bug.

Controller::markedForDeletionAndAbandoned() similarly misjudges
readers(), but that bug does not lead to serious problems, and we would
like to find a fix that identifies such entries without false positives
(that looking at ReadWriteLock::readLevel instead of readers creates).

Also polished Ipc::StoreMap::abortWriting() debugging a little.
---

diff --git a/src/ipc/ReadWriteLock.cc b/src/ipc/ReadWriteLock.cc
index 16afc8aa5f..2e0f57fc23 100644
--- a/src/ipc/ReadWriteLock.cc
+++ b/src/ipc/ReadWriteLock.cc
@@ -127,6 +127,23 @@ Ipc::ReadWriteLock::startAppending()
     appending = true;
 }
 
+bool
+Ipc::ReadWriteLock::stopAppendingAndRestoreExclusive()
+{
+    assert(writing);
+    assert(appending);
+
+    appending = false;
+
+    // Checking `readers` here would mishandle a lockShared() call that started
+    // before we banned appending above, saw still true `appending`, got on a
+    // "success" code path, but had not incremented the `readers` counter yet.
+    // Checking `readLevel` mishandles lockShared() that saw false `appending`,
+    // got on a "failure" code path, but had not decremented `readLevel` yet.
+    // Our callers prefer the wrong "false" to the wrong "true" result.
+    return !readLevel;
+}
+
 void
 Ipc::ReadWriteLock::updateStats(ReadWriteLockStats &stats) const
 {
diff --git a/src/ipc/ReadWriteLock.h b/src/ipc/ReadWriteLock.h
index 36254f3d2a..187d72163a 100644
--- a/src/ipc/ReadWriteLock.h
+++ b/src/ipc/ReadWriteLock.h
@@ -42,6 +42,11 @@ public:
 
     void startAppending(); ///< writer keeps its lock but also allows reading
 
+    /// writer keeps its lock and disallows future readers
+    /// \returns whether access became exclusive (i.e. no readers)
+    /// \prec appending is true
+    bool stopAppendingAndRestoreExclusive();
+
     /// adds approximate current stats to the supplied ones
     void updateStats(ReadWriteLockStats &stats) const;
 
diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index 89d80c8a01..16febcacac 100644
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -253,15 +253,14 @@ Ipc::StoreMap::abortWriting(const sfileno fileno)
     debugs(54, 5, "aborting entry " << fileno << " for writing " << path);
     Anchor &s = anchorAt(fileno);
     assert(s.writing());
-    s.lock.appending = false; // locks out any new readers
-    if (!s.lock.readers) {
+    if (!s.lock.appending || s.lock.stopAppendingAndRestoreExclusive()) {
         freeChain(fileno, s, false);
-        debugs(54, 5, "closed clean entry " << fileno << " for writing " << path);
+        debugs(54, 5, "closed idle entry " << fileno << " for writing " << path);
     } else {
         s.waitingToBeFreed = true;
         s.writerHalted = true;
         s.lock.unlockExclusive();
-        debugs(54, 5, "closed dirty entry " << fileno << " for writing " << path);
+        debugs(54, 5, "closed busy entry " << fileno << " for writing " << path);
     }
 }