[PATCH] Fix an issue where bird may accidently close a socket

[Ondrej Zajicek]

On Mon, May 27, 2019 at 03:29:26PM +0000, Kenth Eriksson wrote:
> On Mon, 2019-05-27 at 17:12 +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 Mon, May 27, 2019 at 02:28:52PM +0200, Kenth Eriksson wrote:
> > > Datagram sockets may return 0 and stream sockets can return 0
> > > if the requested number of bytes to read is 0.
> > 
> > Hi
> > 
> > You mean that if count arg to read() is 0?
> > 
> > How that may happen?
> > 
> We have a client remote controlling bird using a socket that did get
> get POLLHUP, maybe due to that bird closed the socket. 
>
> Don't think checking for 0 is enough, from man page;  

Hi

Did you examine the possible causes of why BIRD closed the socket?
That could be done by adding log messages to sk_read(), perhaps limited
to cases when s->type == SK_UNIX. The man page describes two additional
cases, so it is a good idea to try to distinguish which one happened.

Also, i don't think that check for POLLHUP is correct, as that flag
generally means socket is closed for write, not that it is closed for
read (although some OSes are less consistent in this matter than others).

This is particularly fragile part of code, depends on OS API details, so
i would avoid to do changes there unless we are 100% sure that they are
proper fix for some confirmed condition.


> "When a stream socket peer has performed an orderly shutdown, the
> return value will be 0 (the traditional "end-of-file" return).
> 
> Datagram sockets in various domains (e.g., the UNIX and Internet
> domains) permit zero-length datagrams.  When such a datagram is
> received, the return value is 0.

Well, we open the socket as SOCK_STREAM and not SOCK_DGRAM, so i would
expect this case does not apply. (Although i have no idea what would happen
if the other side opens the socket with SOCK_DGRAM.)

And even if this case would happen, we should ignore it and not fall back
to call_rx_hook() path.


> The value 0 may also be returned if the requested number of bytes to
> receive from a stream socket was 0."

This case means that we called read() with (s->rpos == s->rbuf + s->rbsize),
buffer is full and last call_rx_hook() does not eat data from it. That
means something is wrong, e.g. an incomplete message filled the entire
buffer and without resizing buffer we would just end in endless loop,
therefore closing the connection with error is safest thing to do.

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