<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Dec 31, 2013 at 3:37 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, 25 Dec 2013 17:02:09 +0100 Mariusz Ceier <<a href="mailto:mceier%2Bwayland@gmail.com">mceier+wayland@gmail.com</a>> wrote: > Some structures containing resources list are freed before > resources on that list are destroyed, and that triggers invalid > read/write in compositor.c:3103 indirectly called from > text-backend.c:938 when running weston under valgrind. > > This patch destroys resources on these lists before such > structures are freed. > > That fixes at least x11 and freerds backend. > > Signed-off-by: Mariusz Ceier <<a href="mailto:mceier%2Bwayland@gmail.com">mceier+wayland@gmail.com</a>> > --- > src/compositor.c | 4 ++++ > src/input.c | 15 ++++++++++++--- > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/src/compositor.c b/src/compositor.c > index ff0f3ab..a4077e8 100644 > --- a/src/compositor.c > +++ b/src/compositor.c > @@ -3183,6 +3183,7 @@ weston_compositor_verify_pointers(struct > weston_compositor *ec) WL_EXPORT void > weston_output_destroy(struct weston_output *output) > { > + struct wl_resource *resource, *next_resource; > output->destroying = 1; > > weston_compositor_remove_output(output->compositor, > output); @@ -3192,6 +3193,9 @@ weston_output_destroy(struct > weston_output *output) > wl_signal_emit(&output->destroy_signal, output); > > + wl_resource_for_each_safe(resource, next_resource, > &output->resource_list) > + wl_resource_destroy(resource); > + </div></div>Hi, I didn't check the other cases, but this case seems suspicious at first. There is the following fundamental principle with Wayland protocol objects as I understand it. You cannot just go and destroy wl_resources, because there are clients using them - that is why the wl_resources exist in the first place. Clients use all protocol objects they have, and if the protocol does not say an object is destroyed, then it must be legal to use it, regardless of what happens in the server. If you can guarantee, that clients know the protocol object has gone away without any races, then the clients have already destroyed the protocol objects themselves, which means that you have no wl_resources to destroy in the list. If you cannot guarantee that, then the protocol objects (wl_resources) must continue to exist in such a way, that clients do not get a fatal protocol error when using them. The objects just do nothing. This state is called "inert". If this principle is broken, it usually leads to races between the server and the client, which sometimes leads to a fatal protocol error and causes disconnection of the client. This happens, when the server destroys a wl_resource, and then receives a request from the client for that destroyed resource. The client may have sent the request even before the wl_resource was destroyed in the server, at which point it was a legal request from the client's perspective (this is the essence of the race). However, since wl_output interface contains no requests, your patch might be right. A client cannot send a wl_output request after the wl_resource has been destroyed in the server, because the interface has no requests. The wl_output could still be used as an argument to other requests in other interfaces, though. Whether that is fatal, or just silently gets turned into NULL argument in libwayland-server, I do not remember off-hand. Would be best to verify what happens in that case, since it must not be fatal to the client. </blockquote><div> </div><div>Yes, I just looked it up. If a client sends a request with the output as an argument after the output gets destroyed, the client will get an INVALID_OBJECT protocol error and disconnect. This is a problem. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> If this patch leads to a race with possibly a protocol error as the result (think about output hot-unplug), then I think you would need to turn the wl_resource into an inert object instead of destroying it directly. </blockquote><div> </div><div>The way to do this would be to set dummy handler and call wl_resource_set_destructor(resource, NULL); This leaves the resource inert and allows us to destroy the underlying object. The resources will get cleaned up when the client quits. The reason for the invalid read/write is because the resource destructor calls wl_list_remove on the resource's internal wl_list link. Because we deleted the underlying weston_output object, this results in an invalid read/write. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The same idea applies to the hunks below, too. Thanks, pq <div class="HOEnZb"><div class="h5"> > free(output->name); > pixman_region32_fini(&output->region); > pixman_region32_fini(&output->previous_damage); > diff --git a/src/input.c b/src/input.c > index 07e9d6c..062a2cb 100644 > --- a/src/input.c > +++ b/src/input.c > @@ -471,10 +471,13 @@ weston_pointer_create(struct weston_seat > *seat) WL_EXPORT void > weston_pointer_destroy(struct weston_pointer *pointer) > { > + struct wl_resource *resource, *next_resource; > + > if (pointer->sprite) > pointer_unmap_sprite(pointer); > > - /* XXX: What about pointer->resource_list? */ > + wl_resource_for_each_safe(resource, next_resource, > &pointer->resource_list) > + wl_resource_destroy(resource); > > wl_list_remove(&pointer->focus_resource_listener.link); > wl_list_remove(&pointer->focus_view_listener.link); > @@ -520,7 +523,7 @@ weston_xkb_info_destroy(struct > weston_xkb_info *xkb_info); WL_EXPORT void > weston_keyboard_destroy(struct weston_keyboard *keyboard) > { > - /* XXX: What about keyboard->resource_list? */ > + struct wl_resource *resource, *next_resource; > > #ifdef ENABLE_XKBCOMMON > if (keyboard->seat->compositor->use_xkbcommon) { > @@ -533,6 +536,9 @@ weston_keyboard_destroy(struct > weston_keyboard *keyboard) } > #endif > > + wl_resource_for_each_safe(resource, next_resource, > &keyboard->resource_list) > + wl_resource_destroy(resource); > + > wl_array_release(&keyboard->keys); > wl_list_remove(&keyboard->focus_resource_listener.link); > free(keyboard); > @@ -570,7 +576,10 @@ weston_touch_create(void) > WL_EXPORT void > weston_touch_destroy(struct weston_touch *touch) > { > - /* XXX: What about touch->resource_list? */ > + struct wl_resource *resource, *next_resource; > + > + wl_resource_for_each_safe(resource, next_resource, > &touch->resource_list) > + wl_resource_destroy(resource); > > wl_list_remove(&touch->focus_view_listener.link); > wl_list_remove(&touch->focus_resource_listener.link); > -- > 1.8.5.2 > > _______________________________________________ > wayland-devel mailing list > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> _______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </div></div></blockquote></div> </div></div>