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 d6dfd7410a6598697e8bb67e9230049b7e9691a0..764e71f90950dae7cd924819c642a8ea6a90a879 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -666,6 +666,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 b1301fa7064808be54a8f86a9cc134b72cedbffe..65dbf5efd1a93414b3e7a64c7c726da204879728 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -262,6 +262,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 73efdba17fcd59d6506e1679a5403ef975a9ae7e..a0f95eed00c021e5d6412bb98bc17432b79d511b 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -1343,9 +1343,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. */
         rep->sline.set(rep->sline.version, Http::scPartialContent);
@@ -1724,9 +1722,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 */
@@ -1763,7 +1768,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;
@@ -2545,7 +2550,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 5c0d9fa92a9a18848845956c6f62c96b36e48384..a0ab9d9fd5ebb2ed17272b7354b484f0023b6a8d 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -132,13 +132,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("clientReplyContext::setReplyToStoreEntry"); // removeClientStoreReference() unlocks
     sc = storeClientListAdd(entry, this);
@@ -147,6 +152,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 29f72ac71a500c22cc1876ad6b992d36c4ade723..9dc9f3a12e4094952053e900a5ce18f1002813cd 100644 (file)
--- a/src/client_side_reply.h
+++ b/src/client_side_reply.h
@@ -68,7 +68,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::StatusCode, 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 a81aa288c31a6851bc8b4119eb6862ecced30c40..06c8685047b0779471a153162b7b36af50a203af 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1151,8 +1151,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))
@@ -1819,7 +1818,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 e7f853106b070babb1bc4d516a20de21ca1999a3..95794cc082a1e1a28b4cb822fece795eb262cf49 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1734,8 +1734,7 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
         /* don't cache the result */
         request->flags.cachable = false;
         /* 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;
     }