Re: [PATCH] Consumption of POST body

On 01/18/2013 08:37 PM, Alex Rousskov wrote:
> On 01/18/2013 08:14 AM, Steve Hill wrote:
>
>> The attached patch enables autoconsumption of the BodyPipe if access
>> isn't allowed in order to discard the request body rather than letting
>> it fill the buffers.
>
> Please note that the patch enables auto-consumption for all callout
> errors, not just access denials.
>
> Also, the trunk code is even more complex in this area. I believe your
> patch, when carefully ported to trunk, may enable auto-consumption for
> all callout errors *that have readNextRequest set*. That makes sense to
> me: if we are going to read the next request, we should consume the
> current one without closing the client connection so we should not call
> expectNoForwarding() that may have that side effect.
>
>
> The overall intent sounds correct to me. Even if we do not want to keep
> the connection open after responding with an error, it may be impossible
> to deliver the error message to a half-broken client that does not read
> while writing to us.
>
> However, this brings us back to trunk r11920 and bug 3420. While that
> fix was triggered by an assertion, it is clear from the code comments
> and the commit message that we were worried about a client tying Squid
> client connection "forever" (with or without sending data). Your changes
> will re-enable that behavior in some cases, I guess. More on that below.
>
>
>> I'm not entirely sure this is the best fix - calling
>> expectNoForwarding() would seem to be neater, but this also causes the
>> client-side socket to close, which would break authentication mechanisms
>> that require the connection to be kept alive. Hopefully someone with a
>> better understanding of the Squid internals can review the patch and
>> tell me if it is the right approach?
>
> Thank you very much for treading carefully and disclosing potential
> problems!
>
> Indeed, having two methods with approximately the same purpose and a
> hidden critical distinction is asking for trouble. A lot of existing
> expectNoForwarding-path code comments describe the situation that
> matches yours. Moreover, as far as I can see, we may call the old
> expectNoForwarding() first and then call your forwardToBitbucket()
> later, all for the same error!
>
> As far as I can tell, we have several cases to take care of (or ignore)
> here:
>
> 1. Authentication: Must read next request on the same connection so must
> have readNextRequest set (if not, this is probably a bug). Have to live
> with the possibility of the client sending forever.

In this case we may have problem only for NTLM or similar authentication.
But in this case imagine that the HTTP client post a huge file and Squid
respond with an "401 Unauthorised". Is it normal to read all of the
file in this case? We are going to read all of the file in the next
authenticated request too. So for one request we have to read ALL the
posted file twice. It is not normal.
I believe that the best is to send the error page and abort. The browser
should handle early replies to support such cases...

Also the curl version I have supports early replies, I used custom perl
client to reproduce the problem...

>
> 2. Adaptation errors: The code may have been written to abort the client
> connection, but it also sets readMore or readNextRequest. This sounds
> inconsistent. We should review this, especially since this is the only
> place where we call expectNoForwarding() today!
>
> 3. Other errors: These may or may not have readNextRequest set. Let's
> assume that they set (or not set) that flag correctly.
>
>
> Suggestions:
>
> * Use expectNoForwarding() instead of adding forwardToBitbucket().

This is will work and I believe that this is enough.

>
> * In ConnStateData::noteBodyConsumerAborted() do not stop reading if
> flags.readMore is set. Just keep auto-consuming. This may prevent
> authenticating connections closures even though expectNoForwarding() is
> used. Needs to be tested: Will we correctly stop and switch to the new
> request when the body finally ends?

This will work, but I believe it is not a good policy. It is better to
abort here....

>
> * Discuss and possibly remove readNextRequest setting from
> ClientHttpRequest::handleAdaptationFailure(), to remove inconsistency
> discussed in #2 above.
>
> * Discuss whether we are OK with clients tying up Squid after an
> [authentication] error. Perhaps we need another timeout there, perhaps
> there are already mechanisms to address that, or perhaps we do not
> really care.
>
>
> What do you think?
>
> Alex.
>
>
Received on Mon Jan 21 2013 - 20:15:40 MST