Avoid abandoned client connections after url_rewriter redirects CONNECT. clientProcessRequest() assumes that a CONNECT request is always tunneled and sets flags.readMore to false. However, if url_rewriter redirects the CONNECTing user, Squid responds with a redirect message and does not tunnel. In that case, we do want to read more. Otherwise, keepaliveNextRequest() will declare the connection abandoned and the connection descriptor will "leak" until the connection lifetime expires, even if the client disconnects immediately after receiving the redirect response. The fix delays resets flags.readMore back to true if we call end up calling httpStart() instead of tunnelStart(). My earlier solution was to keep flags.readMore as true until tunnelStart(), but that breaks SslBump because Squid starts reading the [encrypted] client connection (trying to find the next HTTP request there) while obtaining server certificate. This happens because ConnStateData::clientAfterReadingRequests() calls readSomeData() if flags.readMore is true, even if context->mayUseConnection() is also true. The current solution is better because, besides working, it keeps flags.readMore and context->mayUseConnection() consistent. There should be no effect on CONNECT authentication (another case where CONNECT is not tunneled) because the changes should not touch authentication code paths, but that assertion has not been tested. === modified file 'src/client_side.cc' --- src/client_side.cc 2013-01-18 05:57:22 +0000 +++ src/client_side.cc 2013-01-18 21:47:00 +0000 @@ -2762,40 +2762,45 @@ const bool supportedExpect = (expect.caseCmp("100-continue") == 0); if (!supportedExpect) { clientStreamNode *node = context->getClientReplyContext(); clientReplyContext *repContext = dynamic_cast(node->data.getRaw()); assert (repContext); conn->quitAfterError(request); repContext->setReplyToError(ERR_INVALID_REQ, HTTP_EXPECTATION_FAILED, request->method, http->uri, conn->clientConnection->remote, request, NULL, NULL); assert(context->http->out.offset == 0); context->pullData(); goto finish; } } http->request = HTTPMSGLOCK(request); clientSetKeepaliveFlag(http); // Let tunneling code be fully responsible for CONNECT requests if (http->request->method == Http::METHOD_CONNECT) { context->mayUseConnection(true); + // "may" in "mayUse" is meaningful here: no tunnel if Squid redirects, + // needs to authenticate, or responds with an error. For now, pause so + // that we do not read and try to parse tunneled and/or encrypted data. + // We resume reading if we get to ClientHttpRequest::httpStart() or + // ConnStateData::switchToHttps(). conn->flags.readMore = false; } #if USE_SSL if (conn->switchedToHttps() && conn->serveDelayedError(context)) goto finish; #endif /* Do we expect a request-body? */ expectBody = chunked || request->content_length > 0; if (!context->mayUseConnection() && expectBody) { request->body_pipe = conn->expectRequestBody( chunked ? -1 : request->content_length); // consume header early so that body pipe gets just the body connNoteUseOfBuffer(conn, http->req_sz); notedUseOfBuffer = true; /* Is it too large? */ if (!chunked && // if chunked, we will check as we accumulate @@ -4280,40 +4285,41 @@ bool ConnStateData::transparent() const { return clientConnection != NULL && (clientConnection->flags & (COMM_TRANSPARENT|COMM_INTERCEPTION)); } bool ConnStateData::reading() const { return reader != NULL; } void ConnStateData::stopReading() { if (reading()) { comm_read_cancel(clientConnection->fd, reader); reader = NULL; } + flags.readMore = false; } BodyPipe::Pointer ConnStateData::expectRequestBody(int64_t size) { bodyPipe = new BodyPipe(this); if (size >= 0) bodyPipe->setBodySize(size); else startDechunkingRequest(); return bodyPipe; } int64_t ConnStateData::mayNeedToReadMoreBody() const { if (!bodyPipe) return 0; // request without a body or read/produced all body bytes if (!bodyPipe->bodySizeKnown()) === modified file 'src/client_side_request.cc' --- src/client_side_request.cc 2012-12-28 15:23:21 +0000 +++ src/client_side_request.cc 2013-01-18 21:34:47 +0000 @@ -1432,40 +1432,43 @@ sslBumpStart(); return; } #endif logType = LOG_TCP_MISS; getConn()->stopReading(); // tunnels read for themselves tunnelStart(this, &out.size, &al->http.code); return; } httpStart(); } void ClientHttpRequest::httpStart() { PROF_start(httpStart); logType = LOG_TAG_NONE; debugs(85, 4, "ClientHttpRequest::httpStart: " << Format::log_tags[logType] << " for '" << uri << "'"); + // now we know that we are not tunneling or bumping CONNECT + getConn()->flags.readMore = true; // resume (or continue) reading + /* no one should have touched this */ assert(out.offset == 0); /* Use the Stream Luke */ clientStreamNode *node = (clientStreamNode *)client_stream.tail->data; clientStreamRead(node, this, node->readBuffer); PROF_stop(httpStart); } #if USE_SSL void ClientHttpRequest::sslBumpNeed(Ssl::BumpMode mode) { debugs(83, 3, HERE << "sslBump required: "<< Ssl::bumpMode(mode)); sslBumpNeed_ = mode; } // called when comm_write has completed static void SslBumpEstablish(const Comm::ConnectionPointer &, char *, size_t, comm_err_t errflag, int, void *data)