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? > But as we do not support pipelining commands properly anyways, we > could > just fix the issue by this patch. > > > Signed-off-by: Kenth Eriksson <[email protected]> > > --- > > sysdep/unix/main.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c > > index 921115b1..cf5f4a3c 100644 > > --- a/sysdep/unix/main.c > > +++ b/sysdep/unix/main.c > > @@ -403,7 +403,10 @@ cli_get_command(cli *c) > > { > > t++; > > c->rx_pos = c->rx_buf; > > - c->rx_aux = t; > > + if (t < tend) > > + c->rx_aux = t; > > + else > > + c->rx_aux = s->rpos = s->rbuf; > > *d = 0; > > return (d < dend) ? 1 : -1; > > } > > -- > > 2.21.0 > > -- > Elen sila lumenn' omentielvo > > Ondrej 'Santiago' Zajicek (email: [email protected]) > OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, > wwwkeys.pgp.net) > "To err is human -- to blame it on a computer is even more so."
