From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Fri, 19 Nov 2010 02:10:21 +0000 (-0700)
Subject: HTTP Compliance: do not forward TRACE with Max-Forwards: 0 after REQMOD
X-Git-Tag: take1~77
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=34ee6e739e73357ff02d76363844eb177b44401c;p=thirdparty%2Fsquid.git

HTTP Compliance: do not forward TRACE with Max-Forwards: 0 after REQMOD

Before the change, Max-Forwards request value was cached in
HttpRequest::max_forwards member. It was set once in
clientProcessRequest() function. This works fine as long as no request
adaptation is performed. Otherwise original HTTP request may be
replaced with adopted one in ClientHttpRequest::noteAdaptationAnswer()
method and max_forwards value is lost.

This change removes HttpRequest::max_forwards member and gets the value
directly from HttpHeader when needed. This adds another string-to-int
conversion for TRACE and OPTIONS requests, but those are rare, and we
save a little in the other, far more common cases by removing the
HttpRequest::max_forwards member.

Removed assertion from clientReplyContext::traceReply() since it is
called from a single place and the condition is checked right before
the call.

Co-Advisors test cases:
    test_case/rfc2616/maxForwardsZero-TRACE-asterisk
    test_case/rfc2616/maxForwardsZero-TRACE-absolute
---

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 4cd07fdaad..feefc7cd85 100644
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -91,7 +91,6 @@ HttpRequest::init()
     ims = -1;
     imslen = 0;
     lastmod = -1;
-    max_forwards = -1;
     client_addr.SetEmpty();
 #if USE_SQUID_EUI
     client_eui48.clear();
@@ -205,7 +204,6 @@ HttpRequest::clone() const
     // range handled in hdrCacheInit()
     copy->ims = ims;
     copy->imslen = imslen;
-    copy->max_forwards = max_forwards;
     copy->hier = hier; // Is it safe to copy? Should we?
 
     copy->errType = errType;
diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index 4c72c6d266..7d34c93f46 100644
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -177,8 +177,6 @@ public:
 
     int imslen;
 
-    int64_t max_forwards;
-
     Ip::Address client_addr;
 
 #if FOLLOW_X_FORWARDED_FOR
diff --git a/src/client_side.cc b/src/client_side.cc
index 421697e1ea..7fed8fc19c 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2510,10 +2510,8 @@ clientProcessRequest(ConnStateData *conn, HttpParser *hp, ClientSocketContext *c
         unsupportedTe = te.size() && te != "identity";
     } // else implied identity coding
 
-    if (method == METHOD_TRACE || method == METHOD_OPTIONS)
-        request->max_forwards = request->header.getInt64(HDR_MAX_FORWARDS);
-
-    mustReplyToOptions = (method == METHOD_OPTIONS) && (request->max_forwards == 0);
+    mustReplyToOptions = (method == METHOD_OPTIONS) &&
+        (request->header.getInt64(HDR_MAX_FORWARDS) == 0);
     if (!urlCheckRequest(request) || mustReplyToOptions || unsupportedTe) {
         clientStreamNode *node = context->getClientReplyContext();
         clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index ee04056c0b..793d5b828c 100644
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -997,7 +997,6 @@ clientReplyContext::traceReply(clientStreamNode * node)
 {
     clientStreamNode *nextNode = (clientStreamNode *)node->node.next->data;
     StoreIOBuffer localTempBuffer;
-    assert(http->request->max_forwards == 0);
     createStoreEntry(http->request->method, request_flags());
     localTempBuffer.offset = nextNode->readBuffer.offset + headers_sz;
     localTempBuffer.length = nextNode->readBuffer.length;
@@ -1693,7 +1692,7 @@ clientGetMoreData(clientStreamNode * aNode, ClientHttpRequest * http)
     // OPTIONS with Max-Forwards:0 handled in clientProcessRequest()
 
     if (context->http->request->method == METHOD_TRACE) {
-        if (context->http->request->max_forwards == 0) {
+        if (context->http->request->header.getInt64(HDR_MAX_FORWARDS) == 0) {
             context->traceReply(aNode);
             return;
         }
diff --git a/src/url.cc b/src/url.cc
index 7c06675911..db1ecb3215 100644
--- a/src/url.cc
+++ b/src/url.cc
@@ -799,7 +799,7 @@ urlCheckRequest(const HttpRequest * r)
     // we support OPTIONS and TRACE directed at us (with a 501 reply, for now)
     // we also support forwarding OPTIONS and TRACE, except for the *-URI ones
     if (r->method == METHOD_OPTIONS || r->method == METHOD_TRACE)
-        return (r->max_forwards == 0 || r->urlpath != "*");
+        return (r->header.getInt64(HDR_MAX_FORWARDS) == 0 || r->urlpath != "*");
 
     if (r->method == METHOD_PURGE)
         return 1;