------------------------------------------------------------
revno: 12677
revision-id: squid3@treenet.co.nz-20140309023519-t6wmqou7m816tgnl
parent: squid3@treenet.co.nz-20140309021524-wtxyrfoagz95ezwp
author: Alex Rousskov <rousskov@measurement-factory.com>
committer: Amos Jeffries <squid3@treenet.co.nz>
branch nick: 3.3
timestamp: Sat 2014-03-08 19:35:19 -0700
message:
  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.
------------------------------------------------------------
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: squid3@treenet.co.nz-20140309023519-t6wmqou7m816tgnl
# target_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# testament_sha1: 0e0335663a55b0dca989893fb98f81d8d9604572
# timestamp: 2014-03-09 02:37:50 +0000
# source_branch: http://bzr.squid-cache.org/bzr/squid3/3.3
# base_revision_id: squid3@treenet.co.nz-20140309021524-\
#   wtxyrfoagz95ezwp
# 
# Begin patch
=== modified file 'src/HttpRequest.cc'
--- src/HttpRequest.cc	2013-09-11 01:03:45 +0000
+++ src/HttpRequest.cc	2014-03-09 02:35:19 +0000
@@ -658,6 +658,20 @@
     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
 {

=== modified file 'src/HttpRequest.h'
--- src/HttpRequest.h	2013-07-26 12:21:20 +0000
+++ src/HttpRequest.h	2014-03-09 02:35:19 +0000
@@ -246,6 +246,8 @@
      */
     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:

=== modified file 'src/client_side.cc'
--- src/client_side.cc	2013-09-29 17:28:00 +0000
+++ src/client_side.cc	2014-03-09 02:35:19 +0000
@@ -1281,9 +1281,7 @@
          * 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 @@
 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 @@
 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 @@
         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)

=== modified file 'src/client_side_reply.cc'
--- src/client_side_reply.cc	2013-12-15 05:23:23 +0000
+++ src/client_side_reply.cc	2014-03-09 02:35:19 +0000
@@ -134,13 +134,18 @@
 
     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 @@
 #endif
     reqofs = 0;
     reqsize = 0;
+    if (http->request)
+        http->request->ignoreRange(reason);
     flags.storelogiccomplete = 1;
     http->storeEntry(entry);
 }

=== modified file 'src/client_side_reply.h'
--- src/client_side_reply.h	2012-09-10 12:49:35 +0000
+++ src/client_side_reply.h	2014-03-09 02:35:19 +0000
@@ -73,7 +73,7 @@
     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

=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc	2014-03-09 02:10:07 +0000
+++ src/client_side_request.cc	2014-03-09 02:35:19 +0000
@@ -1131,8 +1131,7 @@
     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 @@
             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)

=== modified file 'src/http.cc'
--- src/http.cc	2013-11-21 21:54:29 +0000
+++ src/http.cc	2014-03-09 02:35:19 +0000
@@ -1736,8 +1736,7 @@
         /* 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;
     }