On Tue, Dec 31, 2013 at 3:37 AM, Pekka Paalanen <ppaala...@gmail.com> wrote:
> On Wed, 25 Dec 2013 17:02:09 +0100 > Mariusz Ceier <mceier+wayl...@gmail.com> 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 <mceier+wayl...@gmail.com> > > --- > > 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); > > + > > 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. > 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. > > 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. > 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. > > The same idea applies to the hunks below, too. > > > Thanks, > pq > > > 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 > > 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 >
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel