[PATCH] Destroy resources when destroying input and output

[Pekka Paalanen]

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.

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 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