[PATCH 2/3] Fix issue where cli socket buffer may get full

[Ondrej Zajicek]

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.

But as we do not support pipelining commands  properly anyways, we could
just fix the issue by this patch.

> Signed-off-by: Kenth Eriksson <kenth.eriksson at infinera.com>
> ---
>  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: santiago at 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."