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

[Kenth Eriksson]

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