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

[Kristian Høgsberg]

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