------------------------------------------------------------ revno: 14097 revision-id: squid3@treenet.co.nz-20161009194726-w3w62yrrzzmfe29i parent: squid3@treenet.co.nz-20161009155558-k3n240qhklo0uf2h author: Eduard Bagdasaryan committer: Amos Jeffries branch nick: 3.5 timestamp: Mon 2016-10-10 08:47:26 +1300 message: 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. ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: squid3@treenet.co.nz-20161009194726-w3w62yrrzzmfe29i # target_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # testament_sha1: b6f73d691d6f97a40f1779fd22663a3879928931 # timestamp: 2016-10-09 19:51:01 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/3.5 # base_revision_id: squid3@treenet.co.nz-20161009155558-\ # k3n240qhklo0uf2h # # Begin patch === modified file 'src/HttpReply.cc' --- src/HttpReply.cc 2016-09-23 18:14:24 +0000 +++ src/HttpReply.cc 2016-10-09 19:47:26 +0000 @@ -628,3 +628,11 @@ return newValue; } +bool +HttpReply::olderThan(const HttpReply *them) const +{ + if (!them || !them->date || !date) + return false; + return date < them->date; +} + === modified file 'src/HttpReply.h' --- src/HttpReply.h 2016-09-23 15:28:42 +0000 +++ src/HttpReply.h 2016-10-09 19:47:26 +0000 @@ -112,6 +112,10 @@ 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(); === modified file 'src/LogTags.h' --- src/LogTags.h 2016-01-01 00:14:27 +0000 +++ src/LogTags.h 2016-10-09 19:47:26 +0000 @@ -25,6 +25,7 @@ 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, === modified file 'src/client_side_reply.cc' --- src/client_side_reply.cc 2016-09-23 20:49:24 +0000 +++ src/client_side_reply.cc 2016-10-09 19:47:26 +0000 @@ -388,7 +388,7 @@ 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 @@ // 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 @@ 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 @@ // 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"); - - if (collapsedRevalidation) - http->storeEntry()->clearPublicKeyScope(); - - sendClientUpstreamResponse(); + 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(); + + 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(); } === modified file 'src/http.cc' --- src/http.cc 2016-09-07 15:58:59 +0000 +++ src/http.cc 2016-10-09 19:47:26 +0000 @@ -84,7 +84,8 @@ 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 @@ 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 @@ { 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 @@ 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 @@ 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 @@ /* 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; === modified file 'src/http.h' --- src/http.h 2016-04-01 06:15:31 +0000 +++ src/http.h 2016-10-09 19:47:26 +0000 @@ -112,6 +112,9 @@ 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); };