[PATCH 3/4] wayland-server: Abort if a read from a client gives 0 length

[Pekka Paalanen]

On Tue, 23 Sep 2014 18:18:24 +0200
Karsten Otto <karsten.otto at posteo.de> wrote:

> Am 22.09.2014 um 10:49 schrieb Pekka Paalanen <ppaalanen at gmail.com>:
> 
> > On Thu, 11 Sep 2014 21:42:26 +0200
> > Karsten Otto <karsten.otto at posteo.de> wrote:
> > 
> >> From: Philip Withnall <philip at tecnocode.co.uk>
> >> Date: Fri, 15 Feb 2013 12:57:05 +0000
> >> 
> >> This happens if the socket has been gracefully closed.
> >> 
> >> Signed-off-by: Philip Withnall <philip at tecnocode.co.uk>
> >> Signed-off-by: Karsten Otto <ottoka at posteo.de>
> >> ---
> >> src/wayland-server.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/src/wayland-server.c b/src/wayland-server.c
> >> index 674aeca..83e6f83 100644
> >> --- a/src/wayland-server.c
> >> +++ b/src/wayland-server.c
> >> @@ -260,7 +260,7 @@ wl_client_connection_data(int fd, uint32_t mask, void *data)
> >>    len = 0;
> >>    if (mask & WL_EVENT_READABLE) {
> >>        len = wl_connection_read(connection);
> >> -        if (len < 0 && errno != EAGAIN) {
> >> +        if (len <= 0 && errno != EAGAIN) {
> >>            wl_client_destroy(client);
> >>            return 1;
> >>        }
> > 
> > I don't think this is correct. If wl_connection_read() returns zero, it
> > is because recvmsg() returned zero, which means errno has not been set.
> > If some earlier call caused errno to become EAGAIN, you still won't hit
> > the destroy path.
> > 
> Right, better make the EOF case explicit and change the line to
> 
> if (len == 0 || (len < 0 && errno != EAGAIN))
> 
> > Why do we need this change here anyway? That would be good to explain
> > in the commit message. Does it fix a bug that happens only on BSD?
> > Or is it just avoid one more spin through the poll loop?
> > 
> This change is necessary when using something other than epoll for event I/O handling. 
> 
> Currently execution should never reach the point where recvmsg returns EOF (len == 0). Instead, epoll will detect this and indicate EPOLLHUP, which is handeled a few lines above, closing the connection. However, other event mechanisms may not be able to distinguish EOF and regular readability (e.g. select), or at least not consistently across platforms (e.g. POLLHUP). There is also the potential of half-closed connections (shutdown / POLLRDHUP), though I am not sure this is an issue with wayland. 
> 
> In any case, I believe it is safer to catch the EOF result explicitly here. Without it, I got a nasty infinite loop (OSX with libev select backend).
> 

Ah, excellent explanation. This should definitely be in the commit
message. :-)

> > There is also a hypothetical problem here. If the fd polls readable,
> > but there isn't any data to be read, you would disconnect the client.
> > Arguably that should never happen, but should we trust that? The read
> > is explicitly non-blocking as well.
> > 
> If there is nothing to read from a nonblocking socket, recvmsg and friends should return -1 and set errno to EAGAIN. Unless they are not POSIX conformant, in which case you are in deep waters anyway. There is also the pathological case of a zero-size buffer, but that should be prevented elsewhere.
> 

Oh yeah, I suppose. Ok.


Thanks,
pq