From: Eduard Bagdasaryan Date: Sun, 9 Oct 2016 19:47:26 +0000 (+1300) Subject: HTTP: MUST ignore a [revalidation] response with an older Date header. X-Git-Tag: SQUID_3_5_22~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9ead6b573ccf2b48947ecc891d9ef57894f80070;p=thirdparty%2Fsquid.git HTTP: MUST ignore a [revalidation] response with an older Date header. Before this patch, Squid violated the RFC 7234 section 4 MUST requirement: "When more than one suitable response is stored, a cache MUST use the most recent response (as determined by the Date header field)." This problem may be damaging in cache hierarchies where parent caches may have different responses. Trusting the older response may lead to excessive IMS requests, cache thrashing and other problems. --- diff --git a/src/HttpReply.cc b/src/HttpReply.cc index d94a135bfa..23e8edcab8 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -628,3 +628,11 @@ String HttpReply::removeStaleWarningValues(const String &value) return newValue; } +bool +HttpReply::olderThan(const HttpReply *them) const +{ + if (!them || !them->date || !date) + return false; + return date < them->date; +} + diff --git a/src/HttpReply.h b/src/HttpReply.h index c500b9f4a7..29b4a68517 100644 --- a/src/HttpReply.h +++ b/src/HttpReply.h @@ -112,6 +112,10 @@ public: virtual void hdrCacheInit(); + /// whether our Date header value is smaller than theirs + /// \returns false if any information is missing + bool olderThan(const HttpReply *them) const; + private: /** initialize */ void init(); diff --git a/src/LogTags.h b/src/LogTags.h index 3116933680..579fc0a860 100644 --- a/src/LogTags.h +++ b/src/LogTags.h @@ -25,6 +25,7 @@ typedef enum { LOG_TCP_REFRESH_FAIL_OLD, // refresh from origin failed, stale reply sent LOG_TCP_REFRESH_FAIL_ERR, // refresh from origin failed, error forwarded LOG_TCP_REFRESH_MODIFIED, // refresh from origin replaced existing entry + LOG_TCP_REFRESH_IGNORED, // refresh from origin ignored, stale entry sent LOG_TCP_CLIENT_REFRESH_MISS, LOG_TCP_IMS_HIT, LOG_TCP_SWAPFAIL_MISS, diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 7a5fec1ca6..a7046b86ce 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -388,7 +388,7 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) if (deleting) return; - debugs(88, 3, "handleIMSReply: " << http->storeEntry()->url() << ", " << (long unsigned) result.length << " bytes" ); + debugs(88, 3, http->storeEntry()->url() << ", " << (long unsigned) result.length << " bytes"); if (http->storeEntry() == NULL) return; @@ -403,7 +403,7 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) // request to origin was aborted if (EBIT_TEST(http->storeEntry()->flags, ENTRY_ABORTED)) { - debugs(88, 3, "handleIMSReply: request to origin aborted '" << http->storeEntry()->url() << "', sending old entry to client" ); + debugs(88, 3, "request to origin aborted '" << http->storeEntry()->url() << "', sending old entry to client"); http->logType = LOG_TCP_REFRESH_FAIL_OLD; sendClientOldEntry(); } @@ -423,11 +423,11 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) if (http->request->flags.ims && !old_entry->modifiedSince(http->request->ims, http->request->imslen)) { // forward the 304 from origin - debugs(88, 3, "handleIMSReply: origin replied 304, revalidating existing entry and forwarding 304 to client"); + debugs(88, 3, "origin replied 304, revalidating existing entry and forwarding 304 to client"); sendClientUpstreamResponse(); } else { // send existing entry, it's still valid - debugs(88, 3, "handleIMSReply: origin replied 304, revalidating existing entry and sending " << + debugs(88, 3, "origin replied 304, revalidating existing entry and sending " << old_rep->sline.status() << " to client"); sendClientOldEntry(); } @@ -435,26 +435,37 @@ clientReplyContext::handleIMSReply(StoreIOBuffer result) // origin replied with a non-error code else if (status > Http::scNone && status < Http::scInternalServerError) { - // forward response from origin - http->logType = LOG_TCP_REFRESH_MODIFIED; - debugs(88, 3, "handleIMSReply: origin replied " << status << ", replacing existing entry and forwarding to client"); + const HttpReply *new_rep = http->storeEntry()->getReply(); + // RFC 7234 section 4: a cache MUST use the most recent response + // (as determined by the Date header field) + if (new_rep->olderThan(old_rep)) { + http->logType = LOG_TCP_REFRESH_IGNORED; + debugs(88, 3, "origin replied " << status << + " but with an older date header, sending old entry (" << + old_rep->sline.status() << ") to client"); + sendClientOldEntry(); + } else { + http->logType = LOG_TCP_REFRESH_MODIFIED; + debugs(88, 3, "origin replied " << status << + ", replacing existing entry and forwarding to client"); - if (collapsedRevalidation) - http->storeEntry()->clearPublicKeyScope(); + if (collapsedRevalidation) + http->storeEntry()->clearPublicKeyScope(); - sendClientUpstreamResponse(); + sendClientUpstreamResponse(); + } } // origin replied with an error else if (http->request->flags.failOnValidationError) { http->logType = LOG_TCP_REFRESH_FAIL_ERR; - debugs(88, 3, "handleIMSReply: origin replied with error " << status << + debugs(88, 3, "origin replied with error " << status << ", forwarding to client due to fail_on_validation_err"); sendClientUpstreamResponse(); } else { // ignore and let client have old entry http->logType = LOG_TCP_REFRESH_FAIL_OLD; - debugs(88, 3, "handleIMSReply: origin replied with error " << + debugs(88, 3, "origin replied with error " << status << ", sending old entry (" << old_rep->sline.status() << ") to client"); sendClientOldEntry(); } diff --git a/src/http.cc b/src/http.cc index 4507f5353a..24649aa04d 100644 --- a/src/http.cc +++ b/src/http.cc @@ -84,7 +84,8 @@ void httpHdrAdd(HttpHeader *heads, HttpRequest *request, const AccessLogEntryPoi HttpStateData::HttpStateData(FwdState *theFwdState) : AsyncJob("HttpStateData"), Client(theFwdState), lastChunk(0), header_bytes_read(0), reply_bytes_read(0), - body_bytes_truncated(0), httpChunkDecoder(NULL) + body_bytes_truncated(0), httpChunkDecoder(NULL), + sawDateGoBack(false) { debugs(11,5,HERE << "HttpStateData " << this << " created"); ignoreCacheControl = false; @@ -169,6 +170,14 @@ HttpStateData::httpTimeout(const CommTimeoutCbParams ¶ms) mustStop("HttpStateData::httpTimeout"); } +static StoreEntry * +findPreviouslyCachedEntry(StoreEntry *newEntry) { + assert(newEntry->mem_obj); + return newEntry->mem_obj->request ? + storeGetPublicByRequest(newEntry->mem_obj->request) : + storeGetPublic(newEntry->mem_obj->storeId(), newEntry->mem_obj->method); +} + /// Remove an existing public store entry if the incoming response (to be /// stored in a currently private entry) is going to invalidate it. static void @@ -176,7 +185,6 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status) { int remove = 0; int forbidden = 0; - StoreEntry *pe; // If the incoming response already goes into a public entry, then there is // nothing to remove. This protects ready-for-collapsing entries as well. @@ -235,12 +243,7 @@ httpMaybeRemovePublic(StoreEntry * e, Http::StatusCode status) if (!remove && !forbidden) return; - assert(e->mem_obj); - - if (e->mem_obj->request) - pe = storeGetPublicByRequest(e->mem_obj->request); - else - pe = storeGetPublic(e->mem_obj->storeId(), e->mem_obj->method); + StoreEntry *pe = findPreviouslyCachedEntry(e); if (pe != NULL) { assert(e != pe); @@ -331,6 +334,13 @@ HttpStateData::cacheableReply() return 0; } + // RFC 7234 section 4: a cache MUST use the most recent response + // (as determined by the Date header field) + if (sawDateGoBack) { + debugs(22, 3, "NO because " << *entry << " has an older date header."); + return 0; + } + // Check for Surrogate/1.0 protocol conditions // NP: reverse-proxy traffic our parent server has instructed us never to cache if (surrogateNoStore) { @@ -886,7 +896,10 @@ HttpStateData::haveParsedReplyHeaders() /* Check if object is cacheable or not based on reply code */ debugs(11, 3, "HTTP CODE: " << rep->sline.status()); - if (neighbors_do_private_keys) + if (const StoreEntry *oldEntry = findPreviouslyCachedEntry(entry)) + sawDateGoBack = rep->olderThan(oldEntry->getReply()); + + if (neighbors_do_private_keys && !sawDateGoBack) httpMaybeRemovePublic(entry, rep->sline.status()); bool varyFailure = false; diff --git a/src/http.h b/src/http.h index 65bfccd5e1..74f0e5c30a 100644 --- a/src/http.h +++ b/src/http.h @@ -112,6 +112,9 @@ private: bool peerSupportsConnectionPinning() const; ChunkedCodingParser *httpChunkDecoder; + /// Whether we received a Date header older than that of a matching + /// cached response. + bool sawDateGoBack; private: CBDATA_CLASS2(HttpStateData); };