Miss if a 304 update would exceed reply_header_max_size (#1420)

Fetch the resource unconditionally when a 304 (Not Modified) response to
an internal cache revalidation request grows cached HTTP response
headers beyond the reply_header_max_size limit.

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index 4c19083cc043a2307b680ff02b6836f278edde4a..47597be4bb18f71db6be00136e78a8501c4b3921 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -491,6 +491,12 @@ HttpReply::parseTerminatedPrefix(const char * const terminatedBuf, const size_t
     return 0; // parsed nothing, need more data
 }
 
+size_t
+HttpReply::prefixLen() const
+{
+    return sline.packedLength() + header.len + 2;
+}
+
 void
 HttpReply::configureContentLengthInterpreter(Http::ContentLengthInterpreter &interpreter)
 {

diff --git a/src/HttpReply.h b/src/HttpReply.h
index f19614efa8ee69a96d97b9be64a2ee37f219bd38..cf4cacce4135ebc8fc8a178104a63db207f3e9d5 100644 (file)
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -132,6 +132,10 @@ public:
     /// \retval 0 implies that more data is needed to parse the response prefix
     size_t parseTerminatedPrefix(const char *, size_t);
 
+    /// approximate size of a "status-line CRLF headers CRLF" sequence
+    /// \sa HttpRequest::prefixLen()
+    size_t prefixLen() const;
+
 private:
     /** initialize */
     void init();

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 1b4aea7acff64e0ab77cec089b0d3bac604b5540..cd6ddf9b4bfb85dc7f7aeb35a4ee20a1976df515 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -440,12 +440,18 @@ clientReplyContext::handleIMSReply(const StoreIOBuffer result)
 
     // origin replied 304
     if (status == Http::scNotModified) {
-        http->updateLoggingTags(LOG_TCP_REFRESH_UNMODIFIED);
-        http->request->flags.staleIfHit = false; // old_entry is no longer stale
-
         // TODO: The update may not be instantaneous. Should we wait for its
         // completion to avoid spawning too much client-disassociated work?
-        Store::Root().updateOnNotModified(old_entry, *http->storeEntry());
+        if (!Store::Root().updateOnNotModified(old_entry, *http->storeEntry())) {
+            old_entry->release(true);
+            restoreState();
+            http->updateLoggingTags(LOG_TCP_MISS);
+            processMiss();
+            return;
+        }
+
+        http->updateLoggingTags(LOG_TCP_REFRESH_UNMODIFIED);
+        http->request->flags.staleIfHit = false; // old_entry is no longer stale
 
         // if client sent IMS
         if (http->request->flags.ims && !old_entry->modifiedSince(http->request->ims, http->request->imslen)) {

diff --git a/src/http/StatusLine.cc b/src/http/StatusLine.cc
index 9e03d18d83ebfd9738f28d05faa60488a275f407..a6e09b34d4f6fe92577df1b7b342988616304b8d 100644 (file)
--- a/src/http/StatusLine.cc
+++ b/src/http/StatusLine.cc
@@ -46,9 +46,47 @@ Http::StatusLine::reason() const
     return reason_ ? reason_ : Http::StatusCodeString(status());
 }
 
+size_t
+Http::StatusLine::packedLength() const
+{
+    // Keep in sync with packInto(). TODO: Refactor to avoid code duplication.
+
+    auto packedStatus = status();
+    auto packedReason = reason();
+
+    if (packedStatus == scNone) {
+        packedStatus = scInternalServerError;
+        packedReason = StatusCodeString(packedStatus);
+    }
+
+    // "ICY %3d %s\r\n"
+    if (version.protocol == AnyP::PROTO_ICY) {
+        return
+            + 3 // ICY
+            + 1 // SP
+            + 3 // %3d (packedStatus)
+            + 1 // SP
+            + strlen(packedReason) // %s
+            + 2; // CRLF
+    }
+
+    // "HTTP/%d.%d %3d %s\r\n"
+    return
+        + 4 // HTTP
+        + 1 // "/"
+        + 3 // %d.%d (version.major and version.minor)
+        + 1 // SP
+        + 3 // %3d (packedStatus)
+        + 1 // SP
+        + strlen(packedReason) // %s
+        + 2; // CRLF
+}
+
 void
 Http::StatusLine::packInto(Packable * p) const
 {
+    // Keep in sync with packedLength().
+
     assert(p);
 
     auto packedStatus = status();

diff --git a/src/http/StatusLine.h b/src/http/StatusLine.h
index 061052de37e06d2d151d2f1f78772577b07e4ce5..1122591777cc8978cf6322637d1f1306c72165c3 100644 (file)
--- a/src/http/StatusLine.h
+++ b/src/http/StatusLine.h
@@ -47,6 +47,9 @@ public:
     /// retrieve the reason string for this status line
     const char *reason() const;
 
+    /// expected size of packInto() output
+    size_t packedLength() const;
+
     /// pack fields into a Packable object
     void packInto(Packable *) const;
 

diff --git a/src/peer_digest.cc b/src/peer_digest.cc
index 836dcccdc8b1024ac1df29aeb4e1a8dfe5372cab..e29614afd2c1b87c45c199a04e6d01beb7d85fa9 100644 (file)
--- a/src/peer_digest.cc
+++ b/src/peer_digest.cc
@@ -476,7 +476,10 @@ peerDigestFetchReply(void *data, char *buf, ssize_t size)
 
             assert(fetch->old_entry->mem_obj->request);
 
-            Store::Root().updateOnNotModified(fetch->old_entry, *fetch->entry);
+            if (!Store::Root().updateOnNotModified(fetch->old_entry, *fetch->entry)) {
+                peerDigestFetchAbort(fetch, buf, "header update failure after a 304 response");
+                return -1;
+            }
 
             /* get rid of 304 reply */
             storeUnregister(fetch->sc, fetch->entry, fetch);

diff --git a/src/store.cc b/src/store.cc
index 889512520d7b461b836b6b2c8c7e5de725dce4fd..9083f22308548037cca86144b249ce1ac2403dfe 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -35,6 +35,7 @@
 #include "mgr/StoreIoAction.h"
 #include "repl_modules.h"
 #include "RequestFlags.h"
+#include "sbuf/Stream.h"
 #include "SquidConfig.h"
 #include "StatCounters.h"
 #include "stmem.h"
@@ -1442,8 +1443,14 @@ StoreEntry::updateOnNotModified(const StoreEntry &e304)
     // update reply before calling timestampsSet() below
     const auto &oldReply = mem_obj->freshestReply();
     const auto updatedReply = oldReply.recreateOnNotModified(e304.mem_obj->baseReply());
-    if (updatedReply) // HTTP 304 brought in new information
+    if (updatedReply) { // HTTP 304 brought in new information
+        if (updatedReply->prefixLen() > Config.maxReplyHeaderSize) {
+            throw TextException(ToSBuf("cannot update the cached response because its updated ",
+                                       updatedReply->prefixLen(), "-byte header would exceed ",
+                                       Config.maxReplyHeaderSize, "-byte reply_header_max_size"), Here());
+        }
         mem_obj->updateReply(*updatedReply);
+    }
     // else continue to use the previous update, if any
 
     if (!timestampsSet() && !updatedReply)

diff --git a/src/store/Controller.cc b/src/store/Controller.cc
index 03eaaa15b201f2561283ee2400de62ebcab1249e..66147a63e02d695c397ec9c95892bf3f240a0ef0 100644 (file)
--- a/src/store/Controller.cc
+++ b/src/store/Controller.cc
@@ -691,7 +691,7 @@ Store::Controller::handleIdleEntry(StoreEntry &e)
     // and keep the entry in store_table for its on-disk data
 }
 
-void
+bool
 Store::Controller::updateOnNotModified(StoreEntry *old, StoreEntry &e304)
 {
     Must(old);
@@ -708,13 +708,18 @@ Store::Controller::updateOnNotModified(StoreEntry *old, StoreEntry &e304)
     //         sake, it is best to detect and skip such repeated update calls.
     if (e304.mem_obj->appliedUpdates) {
         debugs(20, 5, "ignored repeated update of " << *old << " with " << e304);
-        return;
+        return true;
     }
     e304.mem_obj->appliedUpdates = true;
 
-    if (!old->updateOnNotModified(e304)) {
-        debugs(20, 5, "updated nothing in " << *old << " with " << e304);
-        return;
+    try {
+        if (!old->updateOnNotModified(e304)) {
+            debugs(20, 5, "updated nothing in " << *old << " with " << e304);
+            return true;
+        }
+    } catch (...) {
+        debugs(20, DBG_IMPORTANT, "ERROR: Failed to update a cached response: " << CurrentException);
+        return false;
     }
 
     if (sharedMemStore && old->mem_status == IN_MEMORY && !EBIT_TEST(old->flags, ENTRY_SPECIAL))
@@ -722,6 +727,8 @@ Store::Controller::updateOnNotModified(StoreEntry *old, StoreEntry &e304)
 
     if (old->swap_dirn > -1)
         swapDir->updateHeaders(old);
+
+    return true;
 }
 
 bool

diff --git a/src/store/Controller.h b/src/store/Controller.h
index 60b22d6ca3266ad37071641d24505513d576530d..dbac1ced619c41dd531685035e40baa032b0c8b9 100644 (file)
--- a/src/store/Controller.h
+++ b/src/store/Controller.h
@@ -89,7 +89,8 @@ public:
     void memoryOut(StoreEntry &, const bool preserveSwappable);
 
     /// using a 304 response, update the old entry (metadata and reply headers)
-    void updateOnNotModified(StoreEntry *old, StoreEntry &e304);
+    /// \returns whether the old entry can be used (and is considered fresh)
+    bool updateOnNotModified(StoreEntry *old, StoreEntry &e304);
 
     /// tries to make the entry available for collapsing future requests
     bool allowCollapsing(StoreEntry *, const RequestFlags &, const HttpRequestMethod &);

diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc
index e34ca4778e40145ea9afb629ca03ee764f0b763d..c0384ec76eb35d0968ae96ca3852fa2f35b5ff8b 100644 (file)
--- a/src/tests/stub_HttpReply.cc
+++ b/src/tests/stub_HttpReply.cc
@@ -25,6 +25,7 @@ bool HttpReply::sanityCheckStartLine(const char *, const size_t, Http::StatusCod
 int HttpReply::httpMsgParseError() STUB_RETVAL(0)
 bool HttpReply::expectingBody(const HttpRequestMethod&, int64_t&) const STUB_RETVAL(false)
 size_t HttpReply::parseTerminatedPrefix(const char *, size_t) STUB_RETVAL(0)
+size_t HttpReply::prefixLen() const STUB_RETVAL(0)
 bool HttpReply::parseFirstLine(const char *, const char *) STUB_RETVAL(false)
 void HttpReply::hdrCacheInit() STUB
 HttpReply * HttpReply::clone() const STUB_RETVAL(nullptr)

diff --git a/src/tests/stub_libhttp.cc b/src/tests/stub_libhttp.cc
index 861b92a70632724ed6dfa451ebc86db73549c483..9192b8579f801b9b0bc6dd183c570a659bf4cccd 100644 (file)
--- a/src/tests/stub_libhttp.cc
+++ b/src/tests/stub_libhttp.cc
@@ -92,6 +92,7 @@ void StatusLine::init() STUB
 void StatusLine::clean() STUB
 void StatusLine::set(const AnyP::ProtocolVersion &, Http::StatusCode, const char *) STUB
 const char *StatusLine::reason() const STUB_RETVAL(nullptr)
+size_t StatusLine::packedLength() const STUB_RETVAL(0)
 void StatusLine::packInto(Packable *) const STUB
 bool StatusLine::parse(const String &, const char *, const char *) STUB_RETVAL(false)
 }

diff --git a/src/tests/stub_libstore.cc b/src/tests/stub_libstore.cc
index 72bf1fa381666ee538e8b59b47e234d7f31a2a2b..3d6dfaaccac3a0ef3d2c85e4b7f144c099236029 100644 (file)
--- a/src/tests/stub_libstore.cc
+++ b/src/tests/stub_libstore.cc
@@ -41,7 +41,7 @@ void Controller::configure() STUB
 void Controller::handleIdleEntry(StoreEntry &) STUB
 void Controller::freeMemorySpace(const int) STUB
 void Controller::memoryOut(StoreEntry &, const bool) STUB
-void Controller::updateOnNotModified(StoreEntry *, StoreEntry &) STUB
+bool Controller::updateOnNotModified(StoreEntry *, StoreEntry &) STUB
 bool Controller::allowCollapsing(StoreEntry *, const RequestFlags &, const HttpRequestMethod &) STUB_RETVAL(false)
 void Controller::addReading(StoreEntry *, const cache_key *) STUB
 void Controller::addWriting(StoreEntry *, const cache_key *) STUB