[PATCH weston 4/8] libweston: Make weston_seat release safe

[Pekka Paalanen]

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

> Ensure the server can safely handle client requests for wl_seat resource
> that have become inert due to weston_seat object release and subsequent
> destruction.
> 
> The clean-up involves, among other things, unsetting the destroyed
> weston_seat object from the user data of wl_seat resources, and handling
> this NULL user data case where required.
> 
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> ---
>  libweston/input.c | 45 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/libweston/input.c b/libweston/input.c
> index 48bcc55c..e4daa56b 100644
> --- a/libweston/input.c
> +++ b/libweston/input.c
> @@ -2412,6 +2412,13 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
>  		 uint32_t id)
>  {
>  	struct weston_seat *seat = wl_resource_get_user_data(resource);
> +	struct weston_pointer *pointer;
> +	struct wl_resource *cr;
> +	struct weston_pointer_client *pointer_client;
> +
> +	if (!seat)
> +		return;
> +
>  	/* We use the pointer_state directly, which means we'll
>  	 * give a wl_pointer if the seat has ever had one - even though
>  	 * the spec explicitly states that this request only takes effect
> @@ -2420,10 +2427,7 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
>  	 * This prevents a race between the compositor sending new
>  	 * capabilities and the client trying to use the old ones.
>  	 */
> -	struct weston_pointer *pointer = seat->pointer_state;
> -	struct wl_resource *cr;
> -	struct weston_pointer_client *pointer_client;
> -
> +	pointer = seat->pointer_state;
>  	if (!pointer)
>  		return;
>  
> @@ -2499,6 +2503,12 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
>  		  uint32_t id)
>  {
>  	struct weston_seat *seat = wl_resource_get_user_data(resource);
> +	struct weston_keyboard *keyboard;
> +	struct wl_resource *cr;
> +
> +	if (!seat)
> +		return;
> +
>  	/* We use the keyboard_state directly, which means we'll
>  	 * give a wl_keyboard if the seat has ever had one - even though
>  	 * the spec explicitly states that this request only takes effect
> @@ -2507,9 +2517,7 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
>  	 * This prevents a race between the compositor sending new
>  	 * capabilities and the client trying to use the old ones.
>  	 */
> -	struct weston_keyboard *keyboard = seat->keyboard_state;
> -	struct wl_resource *cr;
> -
> +	keyboard = seat->keyboard_state;
>  	if (!keyboard)
>  		return;
>  
> @@ -2579,6 +2587,12 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
>  	       uint32_t id)
>  {
>  	struct weston_seat *seat = wl_resource_get_user_data(resource);
> +	struct weston_touch *touch;
> +	struct wl_resource *cr;
> +
> +	if (!seat)
> +		return;
> +
>  	/* We use the touch_state directly, which means we'll
>  	 * give a wl_touch if the seat has ever had one - even though
>  	 * the spec explicitly states that this request only takes effect
> @@ -2587,9 +2601,7 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
>  	 * This prevents a race between the compositor sending new
>  	 * capabilities and the client trying to use the old ones.
>  	 */
> -	struct weston_touch *touch = seat->touch_state;
> -	struct wl_resource *cr;
> -
> +	touch = seat->touch_state;
>  	if (!touch)
>  		return;

Hi,

all the seat_get_*() changes have the same problem that they skip
calling wl_resource_create() which will lead to protocol state
mismatch. These functions need to create inert wl_resources, but
thankfully the earlier patches already take care of handling them
further.

> @@ -3087,6 +3099,19 @@ weston_seat_init(struct weston_seat *seat, struct weston_compositor *ec,
>  WL_EXPORT void
>  weston_seat_release(struct weston_seat *seat)
>  {
> +	struct wl_resource *resource;
> +
> +	wl_resource_for_each(resource, &seat->base_resource_list) {
> +		wl_resource_set_user_data(resource, NULL);
> +	}

Other requests which take wl_seat as argument are:
- wl_data_device_manager.get_data_device
- wl_shell_surface.move
- wl_shell_surface.resize
- wl_shell_surface.set_popup

But there are even more in wayland-protocols, you can find them with
$ git grep 'interface="wl_seat"'

These are unlikely to cope with an inert wl_seat.

Patching weston_desktop_seat_from_seat() will probably take care of a
lot.


> +
> +	wl_resource_for_each(resource, &seat->drag_resource_list) {
> +		wl_resource_set_user_data(resource, NULL);
> +	}
> +
> +	wl_list_remove(&seat->base_resource_list);
> +	wl_list_remove(&seat->drag_resource_list);
> +
>  	wl_list_remove(&seat->link);
>  
>  	if (seat->saved_kbd_focus)

Setting NULL with the drag_resource_list needs more changes in
data-device.c. Looks like the user data of wl_data_device resource is
the weston_seat, so all uses of wl_data_device needs to be audited.
That's all the functions in data_device_interface. Luckily none of the
wl_data_device requests create new protocol objects, and wl_data_device
is never used as a request argument.


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/705a0c29/attachment.sig>