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

[Kenth Eriksson]

On Wed, 2019-05-29 at 16:02 +0200, Ondrej Zajicek wrote:
> 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.
> 
We are still trying to reproduce. 

> 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).
> 
POLLOUT and POLLHUP are mutually exclusive. But POLLIN and POLLHUP are
NOT mutually exclusive. But I can't see that the socket will transition
from POLLHUP|POLLIN to POLLIN only.  

As I see it there are three ways to tell if a socket is closed;

1) write returns EPIPE
2) read returns 0 when requesting to read n bytes where n > 0
3) POLLHUP from poll (irrespective if read or write)

 
> 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.
> 
Unserstood. If we are able to reproduce, we should be able to confirm
or update patch.
> 
> > "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.
>