From: Alex Rousskov Date: Sat, 29 Aug 2015 20:11:19 +0000 (-0700) Subject: When a RESPMOD service aborts, mark the body it produced as truncated. X-Git-Tag: SQUID_3_5_8~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=150fc12b255af4da80bd188076f8d644eaadc26d;p=thirdparty%2Fsquid.git When a RESPMOD service aborts, mark the body it produced as truncated. Without these changes, the recipient of the truncated body often cannot tell that the body was actually truncated (e.g., when Squid uses chunked encoding for body delivery). Lying about truncation may result in rather serious user-level problems. --- diff --git a/src/Store.h b/src/Store.h index 9cabcd83e6..47fe985d0d 100644 --- a/src/Store.h +++ b/src/Store.h @@ -73,6 +73,8 @@ public: } virtual bool isAccepting() const; virtual size_t bytesWanted(Range const aRange, bool ignoreDelayPool = false) const; + /// flags [truncated or too big] entry with ENTRY_BAD_LENGTH and releases it + void lengthWentBad(const char *reason); virtual void complete(); virtual store_client_t storeClientType() const; virtual char const *getSerialisedMetaData(); diff --git a/src/client_side.cc b/src/client_side.cc index 8dcefb163f..3ba425db06 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -963,6 +963,8 @@ ClientSocketContext::lengthToSend(Range const &available) void ClientSocketContext::noteSentBodyBytes(size_t bytes) { + debugs(33, 7, bytes << " body bytes"); + http->out.offset += bytes; if (!http->request->range) diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc index 126815819f..5f05dcf8e1 100644 --- a/src/client_side_reply.cc +++ b/src/client_side_reply.cc @@ -1217,6 +1217,11 @@ clientReplyContext::replyStatus() return STREAM_FAILED; } + if (EBIT_TEST(http->storeEntry()->flags, ENTRY_BAD_LENGTH)) { + debugs(88, 5, "clientReplyStatus: truncated response body"); + return STREAM_UNPLANNED_COMPLETE; + } + if (!done) { debugs(88, 5, "clientReplyStatus: closing, !done, but read 0 bytes"); return STREAM_FAILED; diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 10aa17b3c3..32f3c3a311 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -797,8 +797,22 @@ Client::endAdaptedBodyConsumption() // premature end of the adapted response body void Client::handleAdaptedBodyProducerAborted() { + if (abortOnBadEntry("entry went bad while waiting for the now-aborted adapted body")) + return; + + Must(adaptedBodySource != nullptr); + if (!adaptedBodySource->exhausted()) { + debugs(11,5, "waiting to consume the remainder of the aborted adapted body"); + return; // resumeBodyStorage() should eventually consume the rest + } + stopConsumingFrom(adaptedBodySource); - handleAdaptationAborted(); + + if (handledEarlyAdaptationAbort()) + return; + + entry->lengthWentBad("body adaptation aborted"); + handleAdaptationCompleted(); // the user should get a truncated response } // common part of noteAdaptationAnswer and handleAdaptedBodyProductionEnded @@ -831,18 +845,29 @@ Client::handleAdaptationAborted(bool bypassable) return; // TODO: bypass if possible + if (!handledEarlyAdaptationAbort()) + abortTransaction("adaptation failure with a filled entry"); +} +/// If the store entry is still empty, fully handles adaptation abort, returning +/// true. Otherwise just updates the request error detail and returns false. +bool +Client::handledEarlyAdaptationAbort() +{ if (entry->isEmpty()) { - debugs(11,9, HERE << "creating ICAP error entry after ICAP failure"); + debugs(11,8, "adaptation failure with an empty entry: " << *entry); ErrorState *err = new ErrorState(ERR_ICAP_FAILURE, Http::scInternalServerError, request); err->detailError(ERR_DETAIL_ICAP_RESPMOD_EARLY); fwd->fail(err); fwd->dontRetry(true); - } else if (request) { // update logged info directly - request->detailError(ERR_ICAP_FAILURE, ERR_DETAIL_ICAP_RESPMOD_LATE); + abortTransaction("adaptation failure with an empty entry"); + return true; // handled } - abortTransaction("ICAP failure"); + if (request) // update logged info directly + request->detailError(ERR_ICAP_FAILURE, ERR_DETAIL_ICAP_RESPMOD_LATE); + + return false; // the caller must handle } // adaptation service wants us to deny HTTP client access to this response diff --git a/src/clients/Client.h b/src/clients/Client.h index de55adf1b1..608568f706 100644 --- a/src/clients/Client.h +++ b/src/clients/Client.h @@ -124,6 +124,7 @@ protected: void handleAdaptationCompleted(); void handleAdaptationBlocked(const Adaptation::Answer &answer); void handleAdaptationAborted(bool bypassable = false); + bool handledEarlyAdaptationAbort(); /// called by StoreEntry when it has more buffer space available void resumeBodyStorage(); diff --git a/src/servers/HttpServer.cc b/src/servers/HttpServer.cc index 49c91ce002..1665a12e66 100644 --- a/src/servers/HttpServer.cc +++ b/src/servers/HttpServer.cc @@ -16,6 +16,7 @@ #include "profiler/Profiler.h" #include "servers/forward.h" #include "SquidConfig.h" +#include "Store.h" namespace Http { @@ -145,6 +146,7 @@ Http::Server::handleReply(HttpReply *rep, StoreIOBuffer receivedData) // the last-chunk if there was no error, ignoring responseFinishedOrFailed. const bool mustSendLastChunk = http->request->flags.chunkedReply && !http->request->flags.streamError && + !EBIT_TEST(http->storeEntry()->flags, ENTRY_BAD_LENGTH) && !context->startOfOutput(); const bool responseFinishedOrFailed = !rep && !receivedData.data && diff --git a/src/store.cc b/src/store.cc index 286eeef979..521c9d08f0 100644 --- a/src/store.cc +++ b/src/store.cc @@ -1040,6 +1040,14 @@ storeCheckCachableStats(StoreEntry *sentry) store_check_cachable_hist.yes.Default); } +void +StoreEntry::lengthWentBad(const char *reason) +{ + debugs(20, 3, "because " << reason << ": " << *this); + EBIT_SET(flags, ENTRY_BAD_LENGTH); + releaseRequest(); +} + void StoreEntry::complete() { @@ -1064,10 +1072,8 @@ StoreEntry::complete() assert(mem_status == NOT_IN_MEMORY); - if (!validLength()) { - EBIT_SET(flags, ENTRY_BAD_LENGTH); - releaseRequest(); - } + if (!EBIT_TEST(flags, ENTRY_BAD_LENGTH) && !validLength()) + lengthWentBad("!validLength() in complete()"); #if USE_CACHE_DIGESTS if (mem_obj->request)