Re: [PATCH weston] Fix a crash when a client disconnects.
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 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@lists.freedesktop.org > > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] Fix a crash when a client disconnects.
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 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@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] Fix a crash when a client disconnects.
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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] Fix a crash when a client disconnects.
> > 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. Is there an easy way to test surface.destroy()? Do the existing clients call that, or would I have to write some custom code? ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] Fix a crash when a client disconnects.
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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
