Re: [PATCH] Comm::TcpReceiver - part 1, 2, and 3

On 8/02/2014 11:44 a.m., Alex Rousskov wrote:
> This email is about a couple of issues that I spotted in the big patch
> that need to be addressed but probably do not have a significant effect
> on the discussions about the overall change design.
>
> On 02/05/2014 07:55 AM, Amos Jeffries wrote:
>
>> - memcpy(space(), newContent, sz);
>> + // memmove() allows memory blocks to overlap
>> + // we do assume it handles space()==newContent efficiently
>> + memmove(space(), newContent, sz);
>> appended(sz);
>
> As far as I can tell, the above assumption is wrong for glibc memmove.
> It appears to copy everything even if the two pointers are identical (it
> just switches to the "backward" copying mode for overlapping areas like
> that). However, I could have missed some guard somewhere, before the
> actual memmove code is called. Here are the sources I checked:
>
> https://sourceware.org/git/?p=glibc.git;a=blob;f=string/memmove.c;h=bf7dcc162770503ed62a262f627bfa526fc5a0d7;hb=HEAD
>
> http://libc.sourcearchive.com/documentation/5.4.46/m68k_2memcopy_8h-source.html
>
>
> If we think MemBuf::append() is now going to be used with space() as an
> argument a lot, we should probably add an if-statement and call
> appended() in those cases.
>
> In any case, the comment should be adjusted unless you have reasons to
> believe that assumption is usually true.

Okay. Fixed. Now checking for the space() case which should be common (I
have not tested counts of the two).

>
>> +#if 0 // keep sending responses until existing pipeline finished.
>> +
>> /** \par
>> * We are done with the response, and we are either still receiving request
>> * body (early response!) or have already stopped receiving anything.
>> *
>> * If we are still receiving, then clientParseRequest() below will fail.
>> * (XXX: but then we will call readNextRequest() which may succeed and
>> * execute a smuggled request as we are not done with the current request).
>> *
>> * If we stopped because we got everything, then try the next request.
>> *
>> * If we stopped receiving because of an error, then close now to avoid
>> * getting stuck and to prevent accidental request smuggling.
>> */
>>
>> + // XXX: what if we stopped receiving after pipelined 10 requests and have 6 reply still to send ??
>> + // XXX: if the 10th request has Connection:close indicating no more to read(2)
>> if (const char *reason = conn->stoppedReceiving()) {
>> - debugs(33, 3, HERE << "closing for earlier request error: " << reason);
>> - conn->clientConnection->close();
>> + debugs(33, 3, "closing for earlier request error: " << reason);
>> + conn->tcp->close();
>> return;
>> }
>> +#endif
>
> Why was this code commented out? Yes, it has problems in some corner
> cases (and old XXX), but the check itself was needed in a relatively
> common case: "If we stopped receiving because of an error, then close
> now to avoid getting stuck and to prevent accidental request smuggling."
>

Because there are more common cases than a parse error to have stopped
receiving:
 - Connection:close on a request N positions down the pipeline.
 - client legitimately half-closed after several requests

In the case of a body processing errors we are explicitly calling
tcp->close() or comm_reset_close() from the chunked parser. No smuggling
attack there.

In the case of a processing error parsing a request headers, the full
headers block is left in the buffer. Any attempt to re-parse it will hit
the same error condition.

> After the above code was removed, we are calling clientParseRequest()
> even though we stopped receiving with an error. It that really intentional?

Not really. The intention was to eable processing of the already parsed
and queued pipeline on that connection until they were completed or
something caused full-close to happen.

It should be harmless due to re-parsing hiting the same error condition
(as mentioned above). But just in case Im changing that parse check to:
  if (!conn->stoppedReceiving() && conn->clientParseRequests()).

That way requests (contexts) which are already parsed properly and
queued for handling will be completed, but no more parsed out of the
buffer. This may still cause some legitimate pipelines which simply have
not been parsed before close condition happened to get dropped, but that
hopefully will not be common. At worst the client cna identify the last
serviced request and will have to re-send the others.

>
> FWIW, the question you ask in the new XXXs does not apply to the old
> code because the old code was using stoppedReceiving() for error cases
> only. Regular EOF is not an error. After your changes,
> stoppedReceiving() is used for errors and EOF (and possibly for other
> things), creating the problem you seem to be referring to in the new
> XXXs above.
>
> It is OK to use stopReceiving() for error, EOF, and other cases.
> However, the stoppedReceiving() callers should be checked and probably
> adjusted if you make this change.

Okay.

> The stopReceiving() code itself also
> needs to be adjusted to reflect its new, expanded purpose.
>

How so?

Amos
Received on Sat Feb 08 2014 - 04:46:35 MST