[PATCH] Destroy resources when destroying input and output
Jason Ekstrand
jason at jlekstrand.net
Thu May 8 17:32:12 PDT 2014
On Tue, Dec 31, 2013 at 3:37 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Wed, 25 Dec 2013 17:02:09 +0100
> Mariusz Ceier <mceier+wayland at 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+wayland at 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 at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140508/a9526e2e/attachment-0001.html>
More information about the wayland-devel
mailing list