Re: [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  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 
> > ---
> >  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, >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 

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

[Pekka Paalanen]

On Fri, 26 Jan 2018 18:47:58 +0200
Alexandros Frantzis  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 
> ---
>  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, >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, >drag_resource_list) {
> + wl_resource_set_user_data(resource, NULL);
> + }
> +
> + 

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

[Alexandros Frantzis]

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 
---
 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;
 
@@ -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, >base_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_resource_for_each(resource, >drag_resource_list) {
+   wl_resource_set_user_data(resource, NULL);
+   }
+
+   wl_list_remove(>base_resource_list);
+   wl_list_remove(>drag_resource_list);
+
wl_list_remove(>link);
 
if (seat->saved_kbd_focus)
-- 
2.14.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel