Re: [PATCH] Bug 3420: Request body consumption races and !theConsumer

On 11/17/2011 08:15 PM, Amos Jeffries wrote:
> On 18/11/2011 11:51 a.m., Alex Rousskov wrote:
>>
>> Closed bug 3190, open bug 3420, open bug 3417, and several
>> unreported problems deal with stuck requests, early responses, aborted
>> requests/responses and combination of those. The attached patch attempts
>> to solve many of these mushrooming problems, but it is a significant
>> change that may introduce other bugs. Please review.
>>
>> The patch is against v3.1 because most sufferers are running that
>> version. I will port changes to trunk if approved. Technical details are
>> copied below from the patch preamble.

> Fine. The change itself appears very simple. How much testing has it
> been through?

None beyond very limited lab tests to verify that it solves at least
some of the bugs reported and does not crash under basic requests. At
this time, I do not consider this patch well tested. (Nor do I consider
the change simple, but that is not important :-).

>> TODO: This patch focuses on various error cases. We might still have
>> problems when there is an early HTTP response and no errors of any kind.
>> I marked the corresponding old code with an XXX.
>
> I'm not sure what you are thinking here. Early response with a
> keep-alive should not be hitting the close / stopSending() code path
> unless its an error.

True. The problem starts in ClientSocketContext::keepaliveNextRequest
part of the code which handles the "no error" cases. See the new XXX
added by the patch there.

> And early response with a Connection:close leaving
> the clients side (because server-side pconn is irrelevant) can be closed
> fully just fine after the write.

The problem may exist for early responses on persistent connections,
with no errors of any kind.

> Do we need any clean and purge logics to empty the pipeline without
> errors being set on that early close case?

It is not an "early close" case. Just an "early response" case.

See ClientSocketContext::keepaliveNextRequest(). Assume no errors of any
kind but also assume that we are still reading the request body (i.e.,
that we are dealing with an early response). Observe that the call to
clientParseRequest() will fail to read the next request, as it should.
However, I suspect that we may then call readNextRequest() which might
actually start reading the next request using the current request body.

I may very well be wrong. Somebody should test whether it could happen
like that, using carefully crafted transactions. It will be difficult to
bypass all the protections we already have, but perhaps not impossible.

Conceptually, it is clearly wrong to call clientParseRequest() or
readNextRequest() when we are still not done with the current request.
Those methods are named and written to deal with "next" requests.
Whether current protections in those methods are sufficient to prevent
request smuggling is a question asked by the XXX I added.

A probably much simpler alternative is to add some code to
ClientSocketContext::keepaliveNextRequest() to make sure those two "next
request" methods are not called if we are not done with the current
request AND also check that there is some other code to call those
methods when we are done with the current request.

> The edge case is likely to be
> ClientSocketContext::keepaliveNextRequest() where pinning closes the
> conn. That is one error case and could potentially lead to problems. I
> suspect that it should be doing stopSending("pinned server gone") before
> it does comm_close(). Do you agree?

I am afraid no. There is no point in calling stopSending (or
stopReceiving) before calling comm_close() because comm_close() will
kill the connection and we would have to stop sending and receiving.

> For the commit. Please rename ClientSocketContext::initiateClose to
> closeOnSendError() or something clearer. Or remove entirely. It is only
> used a few times in client_side.cc

Yes, I was thinking about removing it, but wanted to minimize changes in
the patch.

Alex.
Received on Fri Nov 18 2011 - 18:01:53 MST