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

[Ondrej Zajicek]

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