Bug 2821: Ignore Content-Range in non-206 responses (#77)

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.

diff --git a/src/HttpHdrRange.cc b/src/HttpHdrRange.cc
index baf9656e59303eeabb7c90785cc5a6431a9acd9f..5abd799646e38ec2fce2b96b645007211755ea66 100644 (file)
--- 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 d8daf14e8ba045a31069416ce91b12fb9485a970..66e3cbfbec96c9a077a7f41aa74eddebab8efc26 100644 (file)
--- a/src/HttpHeaderRange.h
+++ b/src/HttpHeaderRange.h
@@ -78,7 +78,7 @@ public:
     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<HttpHdrRangeSpec *> specs;
 
 private:

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index 8b5c222519f6e078cac8d330863a041972386d9f..bc448ec51490953e454a19854d96c592fedb3a15 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -25,9 +25,16 @@
 #include "Store.h"
 #include "StrList.h"
 
-HttpReply::HttpReply() : Http::Message(hoReply), date (0), last_modified (0),
-    expires (0), surrogate_control (NULL), content_range (NULL), keep_alive (0),
-    protoPrefix("HTTP/"), bodySizeMax(-2)
+HttpReply::HttpReply():
+    Http::Message(hoReply),
+    date(0),
+    last_modified(0),
+    expires(0),
+    surrogate_control(nullptr),
+    keep_alive(0),
+    protoPrefix("HTTP/"),
+    bodySizeMax(-2),
+    content_range(nullptr)
 {
     init();
 }
@@ -304,7 +311,8 @@ HttpReply::hdrCacheInit()
     date = header.getTime(Http::HdrType::DATE);
     last_modified = header.getTime(Http::HdrType::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(Http::HdrType::CONTENT_TYPE);
 
@@ -317,6 +325,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 c2177e92c713424a270eb4bcc2f03aaa1a193fa0..c2cb78c48c3f9f124398098ffdf134c152787830 100644 (file)
--- a/src/HttpReply.h
+++ b/src/HttpReply.h
@@ -51,7 +51,8 @@ public:
 
     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;
 
@@ -141,6 +142,8 @@ private:
 
     mutable int64_t bodySizeMax; /**< cached result of calcMaxBodySize */
 
+    HttpHdrContRange *content_range; ///< parsed Content-Range; nil for non-206 responses!
+
 protected:
     virtual void packFirstLineInto(Packable * p, bool) const { sline.packInto(p); }
 

diff --git a/src/clients/Client.cc b/src/clients/Client.cc
index 6448b35223145bc89f1a0237cade5b162355040e..642aa5100cc3e0e99e39e4638b7264c208196bbe 100644 (file)
--- a/src/clients/Client.cc
+++ b/src/clients/Client.cc
@@ -507,9 +507,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/http/Stream.cc b/src/http/Stream.cc
index b4c8dc55641203b4fe8cc4318b4d51b318a71cd9..2d34475dc77411fc64b88dda46a85836a2f0fbd4 100644 (file)
--- a/src/http/Stream.cc
+++ b/src/http/Stream.cc
@@ -169,12 +169,12 @@ Http::Stream::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;
@@ -227,14 +227,14 @@ Http::Stream::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, "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?
 
@@ -423,13 +423,20 @@ Http::Stream::buildRangeHeader(HttpReply *rep)
     assert(request->range);
     /* check if we still want to do ranges */
     int64_t roffLimit = request->getRangeOffsetLimit();
+    auto 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(Http::HdrType::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(Http::HdrType::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)
@@ -464,8 +471,9 @@ Http::Stream::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 != nullptr ?
-                                 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;
@@ -476,19 +484,18 @@ Http::Stream::buildRangeHeader(HttpReply *rep)
         /* append appropriate header(s) */
         if (spec_count == 1) {
             if (!replyMatchRequest) {
-                hdr->delById(Http::HdrType::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(Http::HdrType::CONTENT_RANGE)) {
+                if (!contentRange) {
                     /* No content range, so this was a full object we are
                      * sending parts of.
                      */

diff --git a/src/tests/stub_HttpReply.cc b/src/tests/stub_HttpReply.cc
index febf7ddb0c96251bb4e4e92f91ef4ca6b456cd9f..1689e94f5ff422803ad838dd91636d8d11731ba6 100644 (file)
--- a/src/tests/stub_HttpReply.cc
+++ b/src/tests/stub_HttpReply.cc
@@ -13,8 +13,8 @@
 #include "tests/STUB.h"
 
 HttpReply::HttpReply() : Http::Message(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
@@ -30,4 +30,5 @@ HttpReply * HttpReply::clone() const STUB_RETVAL(NULL)
 bool HttpReply::inheritProperties(const Http::Message *aMsg) STUB_RETVAL(false)
 bool HttpReply::updateOnNotModified(HttpReply const*) STUB_RETVAL(false)
 int64_t HttpReply::bodySize(const HttpRequestMethod&) const STUB_RETVAL(0)
+const HttpHdrContRange *HttpReply::contentRange() const STUB_RETVAL(nullptr)