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

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.

> +#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."

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

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. The stopReceiving() code itself also
needs to be adjusted to reflect its new, expanded purpose.

Thank you,

Alex.
Received on Fri Feb 07 2014 - 22:44:31 MST