HTTP Compliance: Do not forward adapted TRACE with Max-Forwards: 0. Before the change, Max-Forwards request header value was cached using the HttpRequest::max_forwards member. The cache was initialized once in clientProcessRequest() function. That worked fine as long as no request adaptation was performed. Adaptation could replace the original HTTP request with the adapted one, losing the max_forwards value. This change removes the HttpRequest::max_forwards member and gets the header value directly from the HttpHeader object as needed. This means we may do a few more string-to-integer conversions for TRACE and OPTIONS requests, but the overhead is negligible and is probably a tiny net win for the common case. The no-cache approach also works better with eCAP adapters that may modify Max-Forwards. Removed assertion from clientReplyContext::traceReply() since the method 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 === modified file 'src/HttpRequest.cc' --- src/HttpRequest.cc 2010-10-28 00:17:18 +0000 +++ src/HttpRequest.cc 2010-11-18 03:55:48 +0000 @@ -74,41 +74,40 @@ HttpRequest::initHTTP(const HttpRequestM } void HttpRequest::init() { method = METHOD_NONE; protocol = PROTO_NONE; urlpath = NULL; login[0] = '\0'; host[0] = '\0'; host_is_numeric = -1; auth_user_request = NULL; pinned_connection = NULL; port = 0; canonical = NULL; memset(&flags, '\0', sizeof(flags)); range = NULL; ims = -1; imslen = 0; lastmod = -1; - max_forwards = -1; client_addr.SetEmpty(); #if USE_SQUID_EUI client_eui48.clear(); client_eui64.clear(); #endif my_addr.SetEmpty(); body_pipe = NULL; // hier dnsWait = -1; errType = ERR_NONE; errDetail = ERR_DETAIL_NONE; peer_login = NULL; // not allocated/deallocated by this class peer_domain = NULL; // not allocated/deallocated by this class vary_headers = NULL; myportname = null_string; tag = null_string; extacl_user = null_string; extacl_passwd = null_string; extacl_log = null_string; extacl_message = null_string; @@ -188,41 +187,40 @@ HttpRequest::clone() const HttpRequest *copy = new HttpRequest(method, protocol, urlpath.termedBuf()); // TODO: move common cloning clone to Msg::copyTo() or copy ctor copy->header.append(&header); copy->hdrCacheInit(); copy->hdr_sz = hdr_sz; copy->http_ver = http_ver; copy->pstate = pstate; // TODO: should we assert a specific state here? copy->body_pipe = body_pipe; strncpy(copy->login, login, sizeof(login)); // MAX_LOGIN_SZ strncpy(copy->host, host, sizeof(host)); // SQUIDHOSTNAMELEN copy->host_addr = host_addr; copy->port = port; // urlPath handled in ctor copy->canonical = canonical ? xstrdup(canonical) : NULL; // 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; // XXX: what to do with copy->peer_login? copy->lastmod = lastmod; copy->vary_headers = vary_headers ? xstrdup(vary_headers) : NULL; // XXX: what to do with copy->peer_domain? copy->myportname = myportname; copy->tag = tag; copy->extacl_user = extacl_user; copy->extacl_passwd = extacl_passwd; copy->extacl_log = extacl_log; copy->extacl_message = extacl_message; assert(copy->inheritProperties(this)); return copy; === modified file 'src/HttpRequest.h' --- src/HttpRequest.h 2010-10-27 23:29:55 +0000 +++ src/HttpRequest.h 2010-11-18 03:55:36 +0000 @@ -160,42 +160,40 @@ private: public: Ip::Address host_addr; AuthUserRequest::Pointer auth_user_request; u_short port; String urlpath; char *canonical; request_flags flags; HttpHdrRange *range; time_t ims; int imslen; - int64_t max_forwards; - Ip::Address client_addr; #if FOLLOW_X_FORWARDED_FOR Ip::Address indirect_client_addr; #endif /* FOLLOW_X_FORWARDED_FOR */ #if USE_SQUID_EUI /* TODO these might be merged into one field if we can reliably map the EUI-48 into EUI-64 there are some OS differences in the upper bytes. */ Eui::Eui48 client_eui48; Eui::Eui64 client_eui64; #endif Ip::Address my_addr; HierarchyLogEntry hier; int dnsWait; ///< sum of DNS lookup delays in milliseconds, for %dt err_type errType; === modified file 'src/client_side.cc' --- src/client_side.cc 2010-10-26 00:17:17 +0000 +++ src/client_side.cc 2010-11-18 03:56:34 +0000 @@ -2479,44 +2479,42 @@ clientProcessRequest(ConnStateData *conn request->client_addr = conn->peer; #if USE_SQUID_EUI request->client_eui48 = conn->peer_eui48; request->client_eui64 = conn->peer_eui64; #endif #if FOLLOW_X_FORWARDED_FOR request->indirect_client_addr = conn->peer; #endif /* FOLLOW_X_FORWARDED_FOR */ request->my_addr = conn->me; request->myportname = conn->port->name; request->http_ver = http_ver; if (request->header.chunked()) { chunked = true; } else if (request->header.has(HDR_TRANSFER_ENCODING)) { const String te = request->header.getList(HDR_TRANSFER_ENCODING); // HTTP/1.1 requires chunking to be the last encoding if there is one 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(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_UNSUP_REQ, HTTP_NOT_IMPLEMENTED, request->method, NULL, conn->peer, request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); conn->flags.readMoreRequests = false; goto finish; } if (!chunked && !clientIsContentLengthValid(request)) { clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); repContext->setReplyToError(ERR_INVALID_REQ, HTTP_LENGTH_REQUIRED, request->method, NULL, === modified file 'src/client_side_reply.cc' --- src/client_side_reply.cc 2010-11-01 00:52:59 +0000 +++ src/client_side_reply.cc 2010-11-18 03:57:37 +0000 @@ -980,41 +980,40 @@ clientReplyContext::purgeDoPurgeHead(Sto * to the client. */ /* FIXME: This doesn't need to go through the store. Simply * push down the client chain */ createStoreEntry(http->request->method, request_flags()); triggerInitialStoreRead(); HttpReply *rep = new HttpReply; rep->setHeaders(purgeStatus, NULL, NULL, 0, 0, -1); http->storeEntry()->replaceHttpReply(rep); http->storeEntry()->complete(); } void 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; localTempBuffer.data = nextNode->readBuffer.data; storeClientCopy(sc, http->storeEntry(), localTempBuffer, SendMoreData, this); http->storeEntry()->releaseRequest(); http->storeEntry()->buffer(); HttpReply *rep = new HttpReply; rep->setHeaders(HTTP_OK, NULL, "text/plain", http->request->prefixLen(), 0, squid_curtime); http->storeEntry()->replaceHttpReply(rep); http->request->swapOut(http->storeEntry()); http->storeEntry()->complete(); } #define SENDING_BODY 0 #define SENDING_HDRSONLY 1 int clientReplyContext::checkTransferDone() { @@ -1676,41 +1675,41 @@ clientGetMoreData(clientStreamNode * aNo /* no cbdatareference, this is only used once, and safely */ if (context->flags.storelogiccomplete) { StoreIOBuffer tempBuffer; tempBuffer.offset = next->readBuffer.offset + context->headers_sz; tempBuffer.length = next->readBuffer.length; tempBuffer.data = next->readBuffer.data; storeClientCopy(context->sc, http->storeEntry(), tempBuffer, clientReplyContext::SendMoreData, context); return; } if (context->http->request->method == METHOD_PURGE) { context->purgeRequest(); return; } // 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; } /* continue forwarding, not finished yet. */ http->logType = LOG_TCP_MISS; context->doGetMoreData(); } else context->identifyStoreObject(); } void clientReplyContext::doGetMoreData() { /* We still have to do store logic processing - vary, cache hit etc */ if (http->storeEntry() != NULL) { /* someone found the object in the cache for us */ StoreIOBuffer localTempBuffer; === modified file 'src/url.cc' --- src/url.cc 2010-09-02 20:46:42 +0000 +++ src/url.cc 2010-11-18 03:58:44 +0000 @@ -782,41 +782,41 @@ int urlCheckRequest(const HttpRequest * r) { int rc = 0; /* protocol "independent" methods * * actually these methods are specific to HTTP: * they are methods we recieve on our HTTP port, * and if we had a FTP listener would not be relevant * there. * * So, we should delegate them to HTTP. The problem is that we * do not have a default protocol from the client side of HTTP. */ if (r->method == METHOD_CONNECT) return 1; // 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; /* does method match the protocol? */ switch (r->protocol) { case PROTO_URN: case PROTO_HTTP: case PROTO_CACHEOBJ: rc = 1; break; case PROTO_FTP: if (r->method == METHOD_PUT) rc = 1;