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

On 19/11/2011 7:01 a.m., Alex Rousskov wrote:
> 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.

Ah, thanks. understood now.

In that scenario I think you are right on that last paragraph. The "some
other code" IMO should be keepAliveNextRequest() itself which I think
should be called when (a) the response is finished, and (b) the request
is finished. Checking that in case (a) the request is finished arriving,
and case (b) either pipeline is active or both request and response are
finished; before kicking off the parse operations on next request. If
one of the two is not finished just exiting, without scheduling anything
while waiting for the call which will happen when that other has finished.

In that line of design we could probably drop the "keepalive" part of
the name. Just calling it nextRequest() and document that it is
responsible for whatever happens to the client connection on successful
request/response completion.

Its assumption that there are no errors seems okay. Since the errors
take other failure code paths. Just this weird excess of request data
being pushed in legitimately with a C-L header or chunking.

>
>> 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.

I was thinking more along the lines of passing the reason for closure.
Its not causing any bad behaviour that I can see.

Amos
Received on Fri Nov 18 2011 - 22:52:43 MST