From 55e1c6e8870690e74628402e39512c00b8e23b35 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Mon, 17 Jul 2023 09:56:11 +0000 Subject: [PATCH] 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. --- src/HttpReply.cc | 6 ++++++ src/HttpReply.h | 4 ++++ src/client_side_reply.cc | 14 ++++++++++---- src/http/StatusLine.cc | 38 +++++++++++++++++++++++++++++++++++++ src/http/StatusLine.h | 3 +++ src/peer_digest.cc | 5 ++++- src/store.cc | 9 ++++++++- src/store/Controller.cc | 17 ++++++++++++----- src/store/Controller.h | 3 ++- src/tests/stub_HttpReply.cc | 1 + src/tests/stub_libhttp.cc | 1 + src/tests/stub_libstore.cc | 2 +- 12 files changed, 90 insertions(+), 13 deletions(-) diff --git a/src/HttpReply.cc b/src/HttpReply.cc index 4c19083cc0..47597be4bb 100644 --- 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 f19614efa8..cf4cacce41 100644 --- 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 1b4aea7acf..cd6ddf9b4b 100644 --- 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 9e03d18d83..a6e09b34d4 100644 --- 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 061052de37..1122591777 100644 --- 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 836dcccdc8..e29614afd2 100644 --- 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 889512520d..9083f22308 100644 --- 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 03eaaa15b2..66147a63e0 100644 --- 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 60b22d6ca3..dbac1ced61 100644 --- 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 e34ca4778e..c0384ec76e 100644 --- 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 861b92a706..9192b8579f 100644 --- 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 72bf1fa381..3d6dfaacca 100644 --- 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 -- 2.39.5