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

[Alexandros Frantzis]

On Wed, Jan 31, 2018 at 03:21:07PM +0200, Pekka Paalanen wrote:
> 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,

Hi Pekka,

thanks for the review.

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

I was misunderstanding how this worked. I will update these to properly
create (inert) resources in v2.

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

Ack, I will take care of these in v2.

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

Ack.

> Thanks,
> pq

Thanks,
Alexandros