From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 10 Feb 2024 20:03:14 +0000 (+0000)
Subject: Fix MemObject.cc:123: "!updatedReply_" assertion (#1666)
X-Git-Tag: SQUID_7_0_1~214
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=39b5a589110901365963432ef23d9022df107e63;p=thirdparty%2Fsquid.git

Fix MemObject.cc:123: "!updatedReply_" assertion (#1666)

    MemStore.cc(506) copyFromShm: entry 1284 copied ...
    FATAL: assertion failed: MemObject.cc:123: "!updatedReply_"

The code loading a response from the shared memory cache incorrectly
assumed that the being-loaded response could not have been updated by an
HTTP 304 (Not Modified) reply and called adjustableBaseReply() that is
banned for updated responses. The goal of that call was to determine
whether the cached response header has been parsed. That determination
can be made without using a method that is banned for updated responses.

StoreClient and clientReplyContext had very similar checks that used the
right/safe approach because their current code did not need an immediate
access to an "adjustable" response. We have updated all external
psParsed checks to reduce chances that this bug resurfaces.
---

diff --git a/src/MemStore.cc b/src/MemStore.cc
index fd329e0702..d0b4cff7e4 100644
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -507,9 +507,9 @@ MemStore::copyFromShm(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
                    " from " << extra.page << '+' << prefixSize);
 
             // parse headers if needed; they might span multiple slices!
-            auto &reply = e.mem().adjustableBaseReply();
-            if (reply.pstate != Http::Message::psParsed) {
+            if (!e.hasParsedReplyHeader()) {
                 httpHeaderParsingBuffer.append(sliceBuf.data, sliceBuf.length);
+                auto &reply = e.mem().adjustableBaseReply();
                 if (reply.parseTerminatedPrefix(httpHeaderParsingBuffer.c_str(), httpHeaderParsingBuffer.length()))
                     httpHeaderParsingBuffer = SBuf(); // we do not need these bytes anymore
             }
@@ -544,7 +544,7 @@ MemStore::copyFromShm(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnc
     debugs(20, 5, "mem-loaded all " << e.mem_obj->endOffset() << '/' <<
            anchor.basics.swap_file_sz << " bytes of " << e);
 
-    if (e.mem().adjustableBaseReply().pstate != Http::Message::psParsed)
+    if (!e.hasParsedReplyHeader())
         throw TextException(ToSBuf("truncated mem-cached headers; accumulated: ", httpHeaderParsingBuffer.length()), Here());
 
     // from StoreEntry::complete()
diff --git a/src/Store.h b/src/Store.h
index 4650456fa4..f00028a176 100644
--- a/src/Store.h
+++ b/src/Store.h
@@ -56,6 +56,9 @@ public:
     /// \see MemObject::freshestReply()
     const HttpReply *hasFreshestReply() const { return mem_obj ? &mem_obj->freshestReply() : nullptr; }
 
+    /// whether this entry has access to [deserialized] [HTTP] response headers
+    bool hasParsedReplyHeader() const;
+
     void write(StoreIOBuffer);
 
     /** Check if the Store entry is empty
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index c52771d2bb..b3c80b6779 100644
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1062,7 +1062,7 @@ clientReplyContext::storeNotOKTransferDone() const
     assert(mem != nullptr);
     assert(http->request != nullptr);
 
-    if (mem->baseReply().pstate != Http::Message::psParsed)
+    if (!http->storeEntry()->hasParsedReplyHeader())
         /* haven't found end of headers yet */
         return 0;
 
diff --git a/src/store.cc b/src/store.cc
index 97d2e87c61..2adaf5c3c6 100644
--- a/src/store.cc
+++ b/src/store.cc
@@ -227,6 +227,19 @@ StoreEntry::bytesWanted (Range<size_t> const aRange, bool ignoreDelayPools) cons
     return mem_obj->mostBytesWanted(aRange.end, ignoreDelayPools);
 }
 
+bool
+StoreEntry::hasParsedReplyHeader() const
+{
+    if (mem_obj) {
+        const auto &reply = mem_obj->baseReply();
+        if (reply.pstate == Http::Message::psParsed) {
+            debugs(20, 7, reply.hdr_sz);
+            return true;
+        }
+    }
+    return false;
+}
+
 bool
 StoreEntry::checkDeferRead(int) const
 {
diff --git a/src/store_client.cc b/src/store_client.cc
index adc3cc1f7c..6abd7b8124 100644
--- a/src/store_client.cc
+++ b/src/store_client.cc
@@ -633,7 +633,7 @@ store_client::handleBodyFromDisk()
     if (!answeredOnce()) {
         // All on-disk responses have HTTP headers. First disk body read(s)
         // include HTTP headers that we must parse (if needed) and skip.
-        const auto haveHttpHeaders = entry->mem_obj->baseReply().pstate == Http::Message::psParsed;
+        const auto haveHttpHeaders = entry->hasParsedReplyHeader();
         if (!haveHttpHeaders && !parseHttpHeadersFromDisk())
             return;
         skipHttpHeadersFromDisk();