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 fd329e0702b2992466c23476654eb78aac7c4b41..d0b4cff7e449db6056d1ac222b33f50191376bd3 100644 (file)
--- 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 4650456fa48a5c0b32516f5937e74d193da41113..f00028a17622b2430beae0c2eb4cb159efa5ca6b 100644 (file)
--- 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 c52771d2bbbfee79fed218a163f959df9bf1c509..b3c80b6779a32d603200d859a3083d63fb602248 100644 (file)
--- 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 97d2e87c610a9912da0444a3499f28185b83237a..2adaf5c3c64f89773894b7590201899718b227d8 100644 (file)
--- 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 adc3cc1f7c6bc450eb7ea6be09364c5941e5c508..6abd7b8124a85d52aebcfbfa9b6766ceaa721728 100644 (file)
--- 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();