[PATCH weston] Fix a crash when a client disconnects.

[Kristian Høgsberg]

On Sun, Jul 22, 2012 at 09:27:33AM -0700, Dima Ryazanov wrote:
> Oh, sorry, I wasn't sure how to reproduce the other case. (Did you see my
> last email? I don't know if I should write a new client, or if I can test
> it with existing ones.)
> I have some time, I'm just not familiar with the code :)

No worries, it was a quick fix.  Mostly, I just didn't want to step on
your toes if you were going to send in an updated patch.  I'm trying
to tie up loose ends before the 0.95 release, so I went ahead and
fixed it.

Kristian

> On Sun, Jul 22, 2012 at 8:45 AM, Kristian Høgsberg <hoegsberg at gmail.com>wrote:
> 
> > On Tue, Jul 10, 2012 at 02:35:10PM -0400, Kristian Høgsberg wrote:
> > > On Tue, Jul 10, 2012 at 09:28:11AM -0700, Dima Ryazanov wrote:
> > > > I see the following use-after-free when a client disconnects:
> > > >
> > > > ==21822== Invalid write of size 8
> > > > ==21822==    at 0x54A8F74: wl_list_remove (wayland-util.c:51)
> > > > ==21822==    by 0x409C87: destroy_frame_callback (compositor.c:1362)
> > > > ==21822==    by 0x56B44E7: destroy_resource (wayland-server.c:427)
> > > > ==21822==    by 0x54A9557: for_each_helper (wayland-util.c:268)
> > > > ==21822==    by 0x54A9593: wl_map_for_each (wayland-util.c:274)
> > > > ==21822==    by 0x56B4640: wl_client_destroy (wayland-server.c:474)
> > > > ==21822==    by 0x56B3E52: wl_client_connection_data
> > (wayland-server.c:231)
> > > > ==21822==    by 0x56B7BD9: wl_event_source_fd_dispatch
> > (event-loop.c:79)
> > > > ==21822==    by 0x56B85AE: wl_event_loop_dispatch (event-loop.c:410)
> > > > ==21822==    by 0x56B5C05: wl_display_run (wayland-server.c:1090)
> > > > ==21822==    by 0x40ECBE: main (compositor.c:3394)
> > > > ==21822==  Address 0xc793d98 is 616 bytes inside a block of size 680
> > free'd
> > > > ==21822==    at 0x4C2A82E: free (in
> > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> > > > ==21822==    by 0x407DAB: destroy_surface (compositor.c:689)
> > > > ==21822==    by 0x56B44E7: destroy_resource (wayland-server.c:427)
> > > > ==21822==    by 0x54A9557: for_each_helper (wayland-util.c:268)
> > > > ==21822==    by 0x54A9593: wl_map_for_each (wayland-util.c:274)
> > > > ==21822==    by 0x56B4640: wl_client_destroy (wayland-server.c:474)
> > > > ==21822==    by 0x56B3E52: wl_client_connection_data
> > (wayland-server.c:231)
> > > > ==21822==    by 0x56B7BD9: wl_event_source_fd_dispatch
> > (event-loop.c:79)
> > > > ==21822==    by 0x56B85AE: wl_event_loop_dispatch (event-loop.c:410)
> > > > ==21822==    by 0x56B5C05: wl_display_run (wayland-server.c:1090)
> > > > ==21822==    by 0x40ECBE: main (compositor.c:3394)
> > > >
> > > > This seems to fix it for me, though I've no idea if it's actually
> > correct.
> > >
> > > It's actually an elegant fix for the case you hit (client disconnect),
> > > since the callback objects on the list will be destroyed by the
> > > resource cleanup, so all we need to to is take the list head out to
> > > prevent the callback objects from accessing that freed memory as
> > > they're destroyed.
> > >
> > > We have a different case to consider though.  When a surface is
> > > explicitly destroyed by calling surface.destroy(), we still need to
> > > destroy pending callbacks, and that case the resource system wont do
> > > it for us.  So we need to iterate through the list and free them
> > > manually.
> >
> > I was hoping you'd have time to implement the above, but I've pushed
> > 1e51fecdf5ade44c77de325679f1e010d8ba4273 now, which should fix the
> > problem and handle the regular destroy case.
> >
> > Kristian
> >
> > >
> > > Kristian
> > >
> > > > ---
> > > >  src/compositor.c |    2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/src/compositor.c b/src/compositor.c
> > > > index 678e0bd..9fe561a 100644
> > > > --- a/src/compositor.c
> > > > +++ b/src/compositor.c
> > > > @@ -686,6 +686,8 @@ destroy_surface(struct wl_resource *resource)
> > > >     if (!region_is_undefined(&surface->input))
> > > >             pixman_region32_fini(&surface->input);
> > > >
> > > > +   wl_list_remove(&surface->frame_callback_list);
> > > > +
> > > >     free(surface);
> > > >  }
> > > >
> > > > --
> > > > 1.7.9.5
> > > >
> > > > _______________________________________________
> > > > wayland-devel mailing list
> > > > wayland-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >