From e11ad242c1c65a00b8ddc21db3658dee702fb5e7 Mon Sep 17 00:00:00 2001 From: Eduard Bagdasaryan Date: Wed, 11 Jul 2018 08:26:48 +0300 Subject: [PATCH] Bug 2821: Ignore Content-Range in non-206 responses (#250) Squid used to honor Content-Range header in HTTP 200 OK (and possibly other non-206) responses, truncating (and possibly enlarging) some response bodies. RFC 7233 declares Content-Range meaningless for standard HTTP status codes other than 206 and 416. Squid now relays meaningless Content-Range as is, without using its value. Why not just strip a meaningless Content-Range header? Squid does not really know whether it is the status code or the header that is "wrong". Let the client figure it out while the server remains responsible. Also ignore Content-Range in 416 (Range Not Satisfiable) responses because that header does not apply to the response body. Also fixed body corruption of (unlikely) multipart 206 responses to single-part Range requests. Valid multipart responses carry no Content-Range (in the primary header), which confused Squid. --- src/HttpHdrRange.cc | 6 +++--- src/HttpHeaderRange.h | 2 +- src/HttpReply.cc | 23 +++++++++++++++++++---- src/HttpReply.h | 5 ++++- src/client_side.cc | 35 +++++++++++++++++++++-------------- src/clients/Client.cc | 5 ++--- src/tests/stub_HttpReply.cc | 5 +++-- 7 files changed, 53 insertions(+), 28 deletions(-) diff --git a/src/HttpHdrRange.cc b/src/HttpHdrRange.cc index 43fd51d5b8c..a3a61f3212d 100644 --- a/src/HttpHdrRange.cc +++ b/src/HttpHdrRange.cc @@ -372,8 +372,8 @@ HttpHdrRange::canonize(HttpReply *rep) { assert(rep); - if (rep->content_range) - clen = rep->content_range->elength; + if (rep->contentRange()) + clen = rep->contentRange()->elength; else clen = rep->content_length; @@ -527,7 +527,7 @@ HttpHdrRange::offsetLimitExceeded(const int64_t limit) const } bool -HttpHdrRange::contains(HttpHdrRangeSpec& r) const +HttpHdrRange::contains(const HttpHdrRangeSpec& r) const { assert(r.length >= 0); HttpHdrRangeSpec::HttpRange rrange(r.offset, r.offset + r.length); diff --git a/src/HttpHeaderRange.h b/src/HttpHeaderRange.h index b64a4623317..b8fa551c290 100644 --- a/src/HttpHeaderRange.h +++ b/src/HttpHeaderRange.h @@ -80,7 +80,7 @@ class HttpHdrRange int64_t firstOffset() const; int64_t lowestOffset(int64_t) const; bool offsetLimitExceeded(const int64_t limit) const; - bool contains(HttpHdrRangeSpec& r) const; + bool contains(const HttpHdrRangeSpec& r) const; std::vector specs; private: diff --git a/src/HttpReply.cc b/src/HttpReply.cc index 183f1bae18b..3d4ffc28678 100644 --- a/src/HttpReply.cc +++ b/src/HttpReply.cc @@ -24,9 +24,16 @@ #include "Store.h" #include "StrList.h" -HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0), - expires (0), surrogate_control (NULL), content_range (NULL), keep_alive (0), - protoPrefix("HTTP/"), bodySizeMax(-2) +HttpReply::HttpReply(): + HttpMsg(hoReply), + date(0), + last_modified(0), + expires(0), + surrogate_control(nullptr), + keep_alive(0), + protoPrefix("HTTP/"), + bodySizeMax(-2), + content_range(nullptr) { init(); } @@ -317,7 +324,8 @@ HttpReply::hdrCacheInit() date = header.getTime(HDR_DATE); last_modified = header.getTime(HDR_LAST_MODIFIED); surrogate_control = header.getSc(); - content_range = header.getContRange(); + content_range = (sline.status() == Http::scPartialContent) ? + header.getContRange() : nullptr; keep_alive = persistent() ? 1 : 0; const char *str = header.getStr(HDR_CONTENT_TYPE); @@ -330,6 +338,13 @@ HttpReply::hdrCacheInit() expires = hdrExpirationTime(); } +const HttpHdrContRange * +HttpReply::contentRange() const +{ + assert(!content_range || sline.status() == Http::scPartialContent); + return content_range; +} + /* sync this routine when you update HttpReply struct */ void HttpReply::hdrCacheClean() diff --git a/src/HttpReply.h b/src/HttpReply.h index 7d48c30d2a7..769e9cb1176 100644 --- a/src/HttpReply.h +++ b/src/HttpReply.h @@ -52,7 +52,8 @@ class HttpReply: public HttpMsg HttpHdrSc *surrogate_control; - HttpHdrContRange *content_range; + /// \returns parsed Content-Range for a 206 response and nil for others + const HttpHdrContRange *contentRange() const; short int keep_alive; @@ -142,6 +143,8 @@ class HttpReply: public HttpMsg mutable int64_t bodySizeMax; /**< cached result of calcMaxBodySize */ + HttpHdrContRange *content_range; ///< parsed Content-Range; nil for non-206 responses! + protected: virtual void packFirstLineInto(Packer * p, bool) const { sline.packInto(p); } diff --git a/src/client_side.cc b/src/client_side.cc index 027ee0d72f5..d374ad19a52 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -1285,13 +1285,20 @@ ClientSocketContext::buildRangeHeader(HttpReply * rep) /* check if we still want to do ranges */ int64_t roffLimit = request->getRangeOffsetLimit(); + const HttpHdrContRange *contentRange = rep ? rep->contentRange() : nullptr; if (!rep) range_err = "no [parse-able] reply"; else if ((rep->sline.status() != Http::scOkay) && (rep->sline.status() != Http::scPartialContent)) range_err = "wrong status code"; - else if (hdr->has(HDR_CONTENT_RANGE)) - range_err = "origin server does ranges"; + else if (rep->sline.status() == Http::scPartialContent) + range_err = "too complex response"; // probably contains what the client needs + else if (rep->sline.status() != Http::scOkay) + range_err = "wrong status code"; + else if (hdr->has(HDR_CONTENT_RANGE)) { + Must(!contentRange); // this is a 200, not 206 response + range_err = "meaningless response"; // the status code or the header is wrong + } else if (rep->content_length < 0) range_err = "unknown length"; else if (rep->content_length != http->memObject()->getReply()->content_length) @@ -1323,8 +1330,9 @@ ClientSocketContext::buildRangeHeader(HttpReply * rep) // web server responded with a valid, but unexpected range. // will (try-to) forward as-is. //TODO: we should cope with multirange request/responses - bool replyMatchRequest = rep->content_range != NULL ? - request->range->contains(rep->content_range->spec) : + // TODO: review, since rep->content_range is always nil here. + bool replyMatchRequest = contentRange != nullptr ? + request->range->contains(contentRange->spec) : true; const int spec_count = http->request->range->specs.size(); int64_t actual_clen = -1; @@ -1336,19 +1344,18 @@ ClientSocketContext::buildRangeHeader(HttpReply * rep) if (spec_count == 1) { if (!replyMatchRequest) { - hdr->delById(HDR_CONTENT_RANGE); - hdr->putContRange(rep->content_range); + hdr->putContRange(contentRange); actual_clen = rep->content_length; //http->range_iter.pos = rep->content_range->spec.begin(); - (*http->range_iter.pos)->offset = rep->content_range->spec.offset; - (*http->range_iter.pos)->length = rep->content_range->spec.length; + (*http->range_iter.pos)->offset = contentRange->spec.offset; + (*http->range_iter.pos)->length = contentRange->spec.length; } else { HttpHdrRange::iterator pos = http->request->range->begin(); assert(*pos); /* append Content-Range */ - if (!hdr->has(HDR_CONTENT_RANGE)) { + if (!contentRange) { /* No content range, so this was a full object we are * sending parts of. */ @@ -1719,12 +1726,12 @@ ClientSocketContext::getNextRangeOffset() const return start; } - } else if (reply && reply->content_range) { + } else if (reply && reply->contentRange()) { /* request does not have ranges, but reply does */ /** \todo FIXME: should use range_iter_pos on reply, as soon as reply->content_range * becomes HttpHdrRange rather than HttpHdrRangeSpec. */ - return http->out.offset + reply->content_range->spec.offset; + return http->out.offset + reply->contentRange()->spec.offset; } return http->out.offset; @@ -1768,14 +1775,14 @@ ClientSocketContext::socketState() // we got everything we wanted from the store return STREAM_COMPLETE; } - } else if (reply && reply->content_range) { + } else if (reply && reply->contentRange()) { /* reply has content-range, but Squid is not managing ranges */ const int64_t &bytesSent = http->out.offset; - const int64_t &bytesExpected = reply->content_range->spec.length; + const int64_t &bytesExpected = reply->contentRange()->spec.length; debugs(33, 7, HERE << "body bytes sent vs. expected: " << bytesSent << " ? " << bytesExpected << " (+" << - reply->content_range->spec.offset << ")"); + reply->contentRange()->spec.offset << ")"); // did we get at least what we expected, based on range specs? diff --git a/src/clients/Client.cc b/src/clients/Client.cc index bfa04f923bd..efbcc8db989 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -523,9 +523,8 @@ Client::haveParsedReplyHeaders() maybePurgeOthers(); // adaptation may overwrite old offset computed using the virgin response - const bool partial = theFinalReply->content_range && - theFinalReply->sline.status() == Http::scPartialContent; - currentOffset = partial ? theFinalReply->content_range->spec.offset : 0; + const bool partial = theFinalReply->contentRange(); + currentOffset = partial ? theFinalReply->contentRange()->spec.offset : 0; } /// whether to prevent caching of an otherwise cachable response diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc index 343d924ec22..1310f9ab4f0 100644 --- a/src/tests/stub_HttpReply.cc +++ b/src/tests/stub_HttpReply.cc @@ -13,8 +13,8 @@ #include "tests/STUB.h" HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0), - expires (0), surrogate_control (NULL), content_range (NULL), keep_alive (0), - protoPrefix("HTTP/"), do_clean(false), bodySizeMax(-2) + expires(0), surrogate_control(nullptr), keep_alive(0), + protoPrefix("HTTP/"), do_clean(false), bodySizeMax(-2), content_range(nullptr) STUB_NOP HttpReply::~HttpReply() STUB void HttpReply::setHeaders(Http::StatusCode status, const char *reason, const char *ctype, int64_t clen, time_t lmt, time_t expires_) STUB @@ -29,4 +29,5 @@ HttpReply::HttpReply() : HttpMsg(hoReply), date (0), last_modified (0), HttpReply * HttpReply::clone() const STUB_RETVAL(NULL) bool HttpReply::inheritProperties(const HttpMsg *aMsg) STUB_RETVAL(false) int64_t HttpReply::bodySize(const HttpRequestMethod&) const STUB_RETVAL(0) + const HttpHdrContRange *HttpReply::contentRange() const STUB_RETVAL(nullptr)