On Tue, Jun 11, 2019 at 03:54:21PM +0000, Kenth Eriksson wrote:
On Tue, 2019-06-11 at 17:03 +0200, Ondrej Zajicek wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi
Now i see where the bug is hidden:
There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length.
This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length.
E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length.
Correct, you still need patch 1/3 to not close socket if you read zero bytes due to socket buffer is full.
Also patch 1/3 is not only relevant for CLI. E.g. what happens if you get a big burst of protocol packets (OSPF, BGP etc.)? That would cause the socket to get closed due to buffer is full?
No, because all other protocols process data from rx-buffer immediately in rx_hook. It is trivial for datagram-based protocols like OSPF or RIP, where one rx_hook call is one datagram. It is a bit more complicated for TCP-based BGP, where BGP message framing is not congruent with read(), but in that case we process the rx-buffer in rx_hook until we have one full message (and we have big enough buffer for one full message) and the we immediately process it from rx_hook. Therefore we have invariant that after rx_hook, there is non-full buffer. This invariant is not satisfied by CLI rx_hook. This is crudely fixed by 3/3, but i think that if we want to throw away some data, it should be done on message boundary. Also, better way to handle 1/3 is to add condition on line io.c line 2211, so that if we have full buffer, we do not try to check POLLIN, so that we do not even call sk_read() in such case instead of try it to handle from inside sk_read(). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."