Re: [PATCH] Avoid abandoned client connections after url_rewriter redirects CONNECT

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.