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

[Kristian Høgsberg]

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.

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