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

[Marek Chalupa]

Yes, in wayland-client it is used this way. wl_connection_read returns
number of bytes buffered in connection after reading (which should be
positive value if reading was OK), otherwise it returns return value of
recvmsg, which allows to distinguish between error and hangup (-1 and  0).
What I was wondering about is if it can happen that recvmsg will contain
only control data (fds). In that case the wl_connection_read would return 0
because the size of connection's buffer would be unchanged. So far I looked
into it it seems this case is not possible, so this patch is:

Reviewed-by: Marek Chalupa <mchqwerty at gmail.com>

Regards,
Marek

On 6 October 2014 02:38, Jason Ekstrand <jason at jlekstrand.net> wrote:

>
>
> On Sun, Oct 5, 2014 at 2:23 PM, Karsten Otto <karsten.otto at posteo.de>
> wrote:
>
>> Am 29.09.2014 um 06:31 schrieb Jason Ekstrand <jason at jlekstrand.net>:
>>
>> >
>> > On Sep 28, 2014 11:49 AM, "Karsten Otto" <karsten.otto at posteo.de>
>> wrote:
>> > >
>> > > From: Philip Withnall <philip at tecnocode.co.uk>
>> > >
>> > > This happens if the socket has been gracefully closed.
>> > >
>> > > [KAO: It prevents a potential infinite loop when using a different
>> > > event handling mechanism than epoll, if said mechanism cannot
>> > > distinguish EOF from regular read (e.g. select).]
>> > > ---
>> > >  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..85741cb 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 || (len < 0 && errno != EAGAIN)) {
>> > >                         wl_client_destroy(client);
>> > >                         return 1;
>> >
>> > It seems to me as if we should push this up the pipe to
>> wl_connection_read so we fix it server-side too.
>> >
>> What do you mean? Calling wl_client_destroy within wl_client_read? Or
>> making sure the connection is properly disposed of in both client and
>> server code?
>>
>> Regarding the first, you would have to pass the wl_client reference to
>> the read function, which would mix API layers unnecessarily. wl_client_read
>> is basically just a convenient wrapper around recvmsg, so it should expose
>> the same result semantics of >0 (OK), == 0 (EOF), and <0 (error), as it
>> already does. It is up to the caller to handle the cases, if only to
>> provide different log messages for error and EOF.
>>
>> Regarding the second, wl_connection_read is part of the wayland
>> private/internal API, and only used in two places: wayland-client.c, which
>> already handles EOF in an appropriate manner, and wayland-server.c, where
>> the proposed patch handles the missing case as well. If anybody else needs
>> to use wl_connection_read within the wayland libraries in the future, they
>> are hopefully aware of the recv result semantics. If necessary, maybe add
>> some documentation to wayland-private.h.
>>
>
> What I meant was to just make wl_connection_read return something
> consistent (< 0 on EOF).  That said, if we're doing it this way in
> wayland-client, then I'm fine with it.  I didn't actually go look.
> --Jason Ekstrand
>
>
>>
>> > >                 }
>> > > --
>> > > 1.9.1
>> > >
>> > > _______________________________________________
>> > > wayland-devel mailing list
>> > > wayland-devel at lists.freedesktop.org
>> > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20141008/a4b21ed8/attachment.html>