Avoid assertions on Range requests that trigger Squid-generated errors.

Added HttpRequest::ignoreRange() to encapsulate range ignoring logic.
Currently the new method only contains the code common among all callers. More
work is needed to check whether further caller homogenization is possible.

Documented that ClientSocketContext::getNextRangeOffset() may sometimes be
called before it is ready to do its job.

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 5e817ed14c263c7cab23459084e7f2ed9c64bdb0..f23602c4eb5c4406547a760afe204c649d41fea5 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -658,6 +658,20 @@ HttpRequest::getRangeOffsetLimit()
     return rangeOffsetLimit;
 }
 
+void
+HttpRequest::ignoreRange(const char *reason)
+{
+    if (range) {
+        debugs(73, 3, static_cast<void*>(range) << " for " << reason);
+        delete range;
+        range = NULL;
+    }
+    // Some callers also reset isRanged but it may not be safe for all callers:
+    // isRanged is used to determine whether a weak ETag comparison is allowed,
+    // and that check should not ignore the Range header if it was present.
+    // TODO: Some callers also delete HDR_RANGE, HDR_REQUEST_RANGE. Should we?
+}
+
 bool
 HttpRequest::canHandle1xx() const
 {

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 8b891100ff0466c0859cdc4d588e2b06910049a7..d3e70fb69b7ad1e051e6a984096d3f33ea3c41d5 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -246,6 +246,8 @@ public:
      */
     CbcPointer<ConnStateData> clientConnectionManager;
 
+    /// forgets about the cached Range header (for a reason)
+    void ignoreRange(const char *reason);
     int64_t getRangeOffsetLimit(); /* the result of this function gets cached in rangeOffsetLimit */
 
 private:

diff --git a/src/client_side.cc b/src/client_side.cc
index 40fe8709e23f642bae27b10e25277da23aee9e86..9adfe9ca264a03b2ce734261d5b41ea1dc0ed501 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1281,9 +1281,7 @@ ClientSocketContext::buildRangeHeader(HttpReply * rep)
          * offset data, but we won't be requesting it.
          * So, we can either re-request, or generate an error
          */
-        debugs(33, 3, "clientBuildRangeHeader: will not do ranges: " << range_err << ".");
-        delete http->request->range;
-        http->request->range = NULL;
+        http->request->ignoreRange(range_err);
     } else {
         /* XXX: TODO: Review, this unconditional set may be wrong. - TODO: review. */
         httpStatusLineSet(&rep->sline, rep->sline.version,
@@ -1662,9 +1660,16 @@ ClientSocketContext::canPackMoreRanges() const
 int64_t
 ClientSocketContext::getNextRangeOffset() const
 {
+    debugs (33, 5, "range: " << http->request->range <<
+            "; http offset " << http->out.offset <<
+            "; reply " << reply);
+
+    // XXX: This method is called from many places, including pullData() which
+    // may be called before prepareReply() [on some Squid-generated errors].
+    // Hence, we may not even know yet whether we should honor/do ranges.
+
     if (http->request->range) {
         /* offset in range specs does not count the prefix of an http msg */
-        debugs (33, 5, "ClientSocketContext::getNextRangeOffset: http offset " << http->out.offset);
         /* check: reply was parsed and range iterator was initialized */
         assert(http->range_iter.valid);
         /* filter out data according to range specs */
@@ -1701,7 +1706,7 @@ ClientSocketContext::getNextRangeOffset() const
 void
 ClientSocketContext::pullData()
 {
-    debugs(33, 5, HERE << clientConnection << " attempting to pull upstream data");
+    debugs(33, 5, reply << " written " << http->out.size << " into " << clientConnection);
 
     /* More data will be coming from the stream. */
     StoreIOBuffer readBuffer;
@@ -2492,7 +2497,7 @@ bool ConnStateData::serveDelayedError(ClientSocketContext *context)
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
         assert(repContext);
         debugs(33, 5, "Responding with delated error for " << http->uri);
-        repContext->setReplyToStoreEntry(sslServerBump->entry);
+        repContext->setReplyToStoreEntry(sslServerBump->entry, "delayed SslBump error");
 
         // save the original request for logging purposes
         if (!context->http->al->request)

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 1d321c8da9ef5a44911b95ce6e7466541e20ef3a..8ea4f2d04161b4daf65d79edbb22afe01f55b8e7 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -134,13 +134,18 @@ void clientReplyContext::setReplyToError(const HttpRequestMethod& method, ErrorS
 
     http->al->http.code = errstate->httpStatus;
 
+    if (http->request)
+        http->request->ignoreRange("responding with a Squid-generated error");
+
     createStoreEntry(method, RequestFlags());
     assert(errstate->callback_data == NULL);
     errorAppendEntry(http->storeEntry(), errstate);
     /* Now the caller reads to get this */
 }
 
-void clientReplyContext::setReplyToStoreEntry(StoreEntry *entry)
+// Assumes that the entry contains an error response without Content-Range.
+// To use with regular entries, make HTTP Range header removal conditional.
+void clientReplyContext::setReplyToStoreEntry(StoreEntry *entry, const char *reason)
 {
     entry->lock(); // removeClientStoreReference() unlocks
     sc = storeClientListAdd(entry, this);
@@ -149,6 +154,8 @@ void clientReplyContext::setReplyToStoreEntry(StoreEntry *entry)
 #endif
     reqofs = 0;
     reqsize = 0;
+    if (http->request)
+        http->request->ignoreRange(reason);
     flags.storelogiccomplete = 1;
     http->storeEntry(entry);
 }

diff --git a/src/client_side_reply.h b/src/client_side_reply.h
index 155b814d3ea438fa3ccd5be54fda6cfe7a311665..dd8d330d738c50b25746a5305c1ac83a2a9d61c7 100644 (file)
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -73,7 +73,7 @@ public:
     int storeOKTransferDone() const;
     int storeNotOKTransferDone() const;
     /// replaces current response store entry with the given one
-    void setReplyToStoreEntry(StoreEntry *e);
+    void setReplyToStoreEntry(StoreEntry *e, const char *reason);
     /// builds error using clientBuildError() and calls setReplyToError() below
     void setReplyToError(err_type, http_status, const HttpRequestMethod&, char const *, Ip::Address &, HttpRequest *, const char *,
 #if USE_AUTH

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 8ef57d0dc3592874213ad7de19a29de648516e85..bfdd7e01ca425f9baa79d44278f54e2507a4f4cf 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1131,8 +1131,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http)
     else {
         req_hdr->delById(HDR_RANGE);
         req_hdr->delById(HDR_REQUEST_RANGE);
-        delete request->range;
-        request->range = NULL;
+        request->ignoreRange("neither HEAD nor GET");
     }
 
     if (req_hdr->has(HDR_AUTHORIZATION))
@@ -1664,7 +1663,7 @@ ClientHttpRequest::doCallouts()
             clientStreamNode *node = (clientStreamNode *)client_stream.tail->prev->data;
             clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
             assert (repContext);
-            repContext->setReplyToStoreEntry(e);
+            repContext->setReplyToStoreEntry(e, "immediate SslBump error");
             errorAppendEntry(e, calloutContext->error);
             calloutContext->error = NULL;
             if (calloutContext->readNextRequest)

diff --git a/src/http.cc b/src/http.cc
index d11e025a23d7c8013d7ded88946f11462ba67606..96ab2d63c7bf55fbf797dfc3cf9b8f4a996b3d68 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1736,8 +1736,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
         /* don't cache the result */
         request->flags.cachable = 0;
         /* pretend it's not a range request */
-        delete request->range;
-        request->range = NULL;
+        request->ignoreRange("want to request the whole object");
         request->flags.isRanged=false;
     }