[PATCH] wayland-client: Treat EOF when reading the wayland socket as an error

[Kristian Høgsberg]

On Tue, Jul 09, 2013 at 06:58:32PM +0100, Neil Roberts wrote:
> Bill Spitzak <spitzak at gmail.com> writes:
> 
> > Maybe it can abort if ...read_events() is called after an error was 
> > encountered. Then bad toolkits will exit, while ones that check for the 
> > error state can do something intelligent.
> 
> This sounds like a nice idea but it might go a bit wrong if there are
> orthogonal bits of code reading the events. For example if the error is
> first detected in eglSwapBuffers() then the first time the application
> will know about it is when it later calls wl_display_dispatch, but that
> will already be the second thing that tries to read the error so it
> would abort.
> 
> That actually highlights a more serious problem. As soon as the error is
> encountered libwayland-client will actually close the socket. That means
> that eglSwapBuffers might cause the socket to close without the
> application having a chance to know about it. It will then try to poll
> on an invalid socket. Or worse still, something might try to open a file
> in the interim and end up reusing the file descriptor. Then it would
> actually poll on a random unrelated file and completely miss the error!

Argh yes, it's bad to close the fd in display_fatal_error(), we need
to keep it open until the application destroys the wl_display.  I was
thinking that maybe that's also why we're not catching POLLHUP from
poll, but it turns out it's much sillier than that:

commit 99b78fdd7f21539a288bfe846542b633756ce163
Author: Kristian Høgsberg <krh at bitplanet.net>
Date:   Tue Jul 9 18:35:06 2013 -0400

    wayland: Don't clear revents until we've checked for G_IO_HUP
    
    https://bugzilla.gnome.org/show_bug.cgi?id=703892

diff --git a/gdk/wayland/gdkeventsource.c b/gdk/wayland/gdkeventsource.c
index cb335bd..4bcae9b 100644
--- a/gdk/wayland/gdkeventsource.c
+++ b/gdk/wayland/gdkeventsource.c
@@ -150,12 +150,12 @@ _gdk_wayland_display_queue_events (GdkDisplay *display)
 
   display_wayland = GDK_WAYLAND_DISPLAY (display);
   source = (GdkWaylandEventSource *) display_wayland->event_source;
+
   if (source->pfd.revents & G_IO_IN)
-    {
-       wl_display_dispatch(display_wayland->wl_display);
-       source->pfd.revents = 0;
-    }
+    wl_display_dispatch (display_wayland->wl_display);
 
   if (source->pfd.revents & (G_IO_ERR | G_IO_HUP))
     g_error ("Lost connection to wayland compositor");
+
+  source->pfd.revents = 0;
 }

but your patches (wayland and gtk+) still make sense of course.  I
pushed your wayland patch with Robs wording tweak and my follow-up fix
to not return -1 when we get EAGAIN, and I pushed both your gtk+ fix
and the above gtk+ fix.  And both gtk+ fixes are necessary - we can
get G_IO_IN | G_IO_HUP, in which case wl_display_dispatch() will read
whatever data we got up until the HUP and return 0 and the G_IO_HUP
test will the catch the error.  Or we may get another error from
wl_display_dispatch() that isn't reflected in the revents flags.

Kristian

> Egads,
> - Neil
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel