[PATCH weston 1/8] libweston: Make weston_pointer destruction safe

[Pekka Paalanen]

On Fri, 26 Jan 2018 18:47:55 +0200
Alexandros Frantzis <alexandros.frantzis at collabora.com> wrote:

> Properly clean up all sub-objects (e.g., weston_pointer_client objects)
> when a weston_pointer object is destroyed. The clean-up ensures that the
> server is able to safely handle client requests to any associated
> pointer resources, which, as a consenquence of a weston_pointer
> destruction, have now become inert.
> 
> The clean-up involves, among other things, unsetting the destroyed
> weston_pointer object from the user data of pointer resources, and
> handling this NULL user data case where required.
> 
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
>  libweston/input.c | 41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)

Hi Alexandros,

thank you for tackling these very old oversights.

> diff --git a/libweston/input.c b/libweston/input.c
> index 94a3423a..01f0d568 100644
> --- a/libweston/input.c
> +++ b/libweston/input.c
> @@ -105,6 +105,19 @@ weston_pointer_client_create(struct wl_client *client)
>  static void
>  weston_pointer_client_destroy(struct weston_pointer_client *pointer_client)
>  {
> +	struct wl_resource *resource;
> +
> +	wl_resource_for_each(resource, &pointer_client->pointer_resources) {
> +		wl_resource_set_user_data(resource, NULL);
> +	}
> +
> +	wl_resource_for_each(resource,
> +			     &pointer_client->relative_pointer_resources) {
> +		wl_resource_set_user_data(resource, NULL);
> +	}
> +
> +	wl_list_remove(&pointer_client->pointer_resources);
> +	wl_list_remove(&pointer_client->relative_pointer_resources);
>  	free(pointer_client);
>  }
>  
> @@ -170,11 +183,14 @@ unbind_pointer_client_resource(struct wl_resource *resource)
>  	struct wl_client *client = wl_resource_get_client(resource);
>  	struct weston_pointer_client *pointer_client;
>  
> -	pointer_client = weston_pointer_get_pointer_client(pointer, client);
> -	assert(pointer_client);
> -
>  	wl_list_remove(wl_resource_get_link(resource));
> -	weston_pointer_cleanup_pointer_client(pointer, pointer_client);
> +
> +	if (pointer) {
> +		pointer_client = weston_pointer_get_pointer_client(pointer,
> +								   client);
> +		assert(pointer_client);
> +		weston_pointer_cleanup_pointer_client(pointer, pointer_client);
> +	}
>  }
>  
>  static void unbind_resource(struct wl_resource *resource)
> @@ -1092,12 +1108,18 @@ weston_pointer_create(struct weston_seat *seat)
>  WL_EXPORT void
>  weston_pointer_destroy(struct weston_pointer *pointer)
>  {
> +	struct weston_pointer_client *pointer_client, *tmp;
> +
>  	wl_signal_emit(&pointer->destroy_signal, pointer);
>  
>  	if (pointer->sprite)
>  		pointer_unmap_sprite(pointer);
>  
> -	/* XXX: What about pointer->resource_list? */
> +	wl_list_for_each_safe(pointer_client, tmp, &pointer->pointer_clients,
> +			      link) {
> +		wl_list_remove(&pointer_client->link);
> +		weston_pointer_client_destroy(pointer_client);
> +	}
>  
>  	wl_list_remove(&pointer->focus_resource_listener.link);
>  	wl_list_remove(&pointer->focus_view_listener.link);
> @@ -2297,6 +2319,9 @@ pointer_set_cursor(struct wl_client *client, struct wl_resource *resource,
>  	struct weston_pointer *pointer = wl_resource_get_user_data(resource);
>  	struct weston_surface *surface = NULL;
>  
> +	if (!pointer)
> +		return;
> +

Ideally if a wl_surface is provided, we would still do the role check
and assignment, but I see that would require quite some rearranging
here, so this is ok.

>  	if (surface_resource)
>  		surface = wl_resource_get_user_data(surface_resource);
>  
> @@ -3674,6 +3699,9 @@ pointer_constraints_lock_pointer(struct wl_client *client,
>  	struct weston_region *region = region_resource ?
>  		wl_resource_get_user_data(region_resource) : NULL;
>  
> +	if (!pointer)
> +		return;

This doesn't work, because the request is creating a new protocol
object. If you omit creating the zwp_locked_pointer_v1 protocol object
(wl_resource), it will lead to the client and server disagreeing on
what object ids are used and eventually to a fatal protocol error.

> +
>  	init_pointer_constraint(resource, id, surface, pointer, region, lifetime,
>  				&zwp_locked_pointer_v1_interface,
>  				&locked_pointer_interface,
> @@ -4467,6 +4495,9 @@ pointer_constraints_confine_pointer(struct wl_client *client,
>  	struct weston_region *region = region_resource ?
>  		wl_resource_get_user_data(region_resource) : NULL;
>  
> +	if (!pointer)
> +		return;

This one has the same problem as above.

Maybe we should have a patch before this one to make
init_pointer_constraint() deal with pointer=NULL?

> +
>  	init_pointer_constraint(resource, id, surface, pointer, region, lifetime,
>  				&zwp_confined_pointer_v1_interface,
>  				&confined_pointer_interface,

Otherwise this patch looks exactly correct.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180131/35b0a8b9/attachment-0001.sig>