On 01/18/2013 05:32 AM, Amos Jeffries wrote:
> On 17/01/2013 6:47 a.m., Alex Rousskov wrote:
>> Hello,
>>
>> 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 setting flags.readMore to false until we are about to
>> call tunnelStart().
>>
>> The effect on CONNECT authentication (another case where CONNECT is not
>> tunneled) is untested, but I hope that code continues to work because it
>> should be OK with reading more requests on the [being] authenticated
>> connection.
>>
>> These changes may also fix other similar not-tunneled CONNECT cases.
>> They are not related to SslBump.
>>
>>
>> One of the attached patches is for trunk. The other one is for v3.2. I
>> did not check whether the problem exists in v3.1.
>>
>>
>> HTH,
>>
>> Alex.
> +1. It looks okay, although I would like confirmation from somebody
> using NTLM authentication that things still work afterwards.
My patches broke SslBump -- Squid started reading from the client
connection before switching to SSL :-(.
The attached patches were tested with SslBump (and without). They are
more conservative/less invasive: I now leave readMore set to false for
CONNECT until it is clear that Squid is not going to tunnel or bump.
This should break fewer cases.
Alex.
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<clientReplyContext *>(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)
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 2012-11-06 12:45:17 +0000
+++ src/client_side.cc 2013-01-18 22:13:55 +0000
@@ -2629,40 +2629,45 @@
const bool supportedExpect = (expect.caseCmp("100-continue") == 0);
if (!supportedExpect) {
clientStreamNode *node = context->getClientReplyContext();
clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
assert (repContext);
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();
conn->flags.readMore = false;
goto finish;
}
}
http->request = HTTPMSGLOCK(request);
clientSetKeepaliveFlag(http);
// Let tunneling code be fully responsible for CONNECT requests
if (http->request->method == 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;
}
/* 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
clientIsRequestBodyTooLargeForPolicy(request->content_length)) {
clientStreamNode *node = context->getClientReplyContext();
clientReplyContext *repContext = dynamic_cast<clientReplyContext *>(node->data.getRaw());
assert (repContext);
repContext->setReplyToError(ERR_TOO_BIG,
@@ -3873,40 +3878,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
=== modified file 'src/client_side_request.cc'
--- src/client_side_request.cc 2012-10-17 06:47:41 +0000
+++ src/client_side_request.cc 2013-01-18 22:13:55 +0000
@@ -1329,40 +1329,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
bool
ClientHttpRequest::sslBumpNeeded() const
{
assert(sslBumpNeed != needUnknown);
return (sslBumpNeed == needConfirmed);
}
void
ClientHttpRequest::sslBumpNeeded(bool isNeeded)
{
