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

[Dima Ryazanov]

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 :)

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20120722/13e06e7e/attachment.html>