[weston] Fix a crash when unlocking or unconfining a pointer

Submitted by Dima Ryazanov on May 10, 2018, 7:53 a.m.

Details

Message ID 20180510075338.30125-1-dima@gmail.com
State Accepted
Commit e0dc5d47cb5f29deec495efd958fcd5f6f833389
Headers show
Series "Fix a crash when unlocking or unconfining a pointer" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Dima Ryazanov May 10, 2018, 7:53 a.m.
In GNOME (but not in Weston), if a window loses focus, the client first receives
the focus event, then the unlock/unconfine event. This causes toytoolkit to
dereference a NULL window when unlocking or unconfining the pointer.

To repro:
- Run weston-confine
- Click the window
- Alt-Tab away from it

Result:

[1606837.869] wl_keyboard@19.modifiers(63944, 524352, 0, 0, 0)
[1606837.926] wl_keyboard@19.leave(63945, wl_surface@15)
[1606837.945] wl_pointer@18.leave(63946, wl_surface@15)
[1606837.956] wl_pointer@18.frame()
[1606837.961] zwp_confined_pointer_v1@26.unconfined()
Segmentation fault (core dumped)

To fix this, get the input from the window instead of the other way around.

Signed-off-by: Dima Ryazanov <dima@gmail.com>
---
 clients/window.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/clients/window.c b/clients/window.c
index bcf2b017..dee4455f 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -286,6 +286,7 @@  struct window {
 	confined_pointer_unconfined_handler_t pointer_unconfined_handler;
 
 	struct zwp_confined_pointer_v1 *confined_pointer;
+	struct input *confined_input;
 	struct widget *confined_widget;
 	bool confined;
 
@@ -4788,8 +4789,8 @@  static void
 locked_pointer_locked(void *data,
 		      struct zwp_locked_pointer_v1 *locked_pointer)
 {
-	struct input *input = data;
-	struct window *window = input->pointer_focus;
+	struct window *window = data;
+	struct input *input = window->locked_input;
 
 	window->pointer_locked = true;
 
@@ -4804,8 +4805,8 @@  static void
 locked_pointer_unlocked(void *data,
 			struct zwp_locked_pointer_v1 *locked_pointer)
 {
-	struct input *input = data;
-	struct window *window = input->pointer_focus;
+	struct window *window = data;
+	struct input *input = window->locked_input;
 
 	window_unlock_pointer(window);
 
@@ -4860,7 +4861,7 @@  window_lock_pointer(struct window *window, struct input *input)
 							ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_ONESHOT);
 	zwp_locked_pointer_v1_add_listener(locked_pointer,
 					   &locked_pointer_listener,
-					   input);
+					   window);
 
 	window->locked_input = input;
 	window->locked_pointer = locked_pointer;
@@ -4902,8 +4903,8 @@  static void
 confined_pointer_confined(void *data,
 			  struct zwp_confined_pointer_v1 *confined_pointer)
 {
-	struct input *input = data;
-	struct window *window = input->pointer_focus;
+	struct window *window = data;
+	struct input *input = window->confined_input;
 
 	window->confined = true;
 
@@ -4918,8 +4919,8 @@  static void
 confined_pointer_unconfined(void *data,
 			    struct zwp_confined_pointer_v1 *confined_pointer)
 {
-	struct input *input = data;
-	struct window *window = input->pointer_focus;
+	struct window *window = data;
+	struct input *input = window->confined_input;
 
 	window_unconfine_pointer(window);
 
@@ -4984,8 +4985,9 @@  window_confine_pointer_to_rectangles(struct window *window,
 
 	zwp_confined_pointer_v1_add_listener(confined_pointer,
 					     &confined_pointer_listener,
-					     input);
+					     window);
 
+	window->confined_input = input;
 	window->confined_pointer = confined_pointer;
 	window->confined_widget = NULL;
 
@@ -5046,6 +5048,7 @@  window_unconfine_pointer(struct window *window)
 	zwp_confined_pointer_v1_destroy(window->confined_pointer);
 	window->confined_pointer = NULL;
 	window->confined = false;
+	window->confined_input = NULL;
 }
 
 static void

Comments

On Thu, 10 May 2018 00:53:38 -0700
Dima Ryazanov <dima@gmail.com> wrote:

> In GNOME (but not in Weston), if a window loses focus, the client first receives
> the focus event, then the unlock/unconfine event. This causes toytoolkit to
> dereference a NULL window when unlocking or unconfining the pointer.
> 
> To repro:
> - Run weston-confine
> - Click the window
> - Alt-Tab away from it
> 
> Result:
> 
> [1606837.869] wl_keyboard@19.modifiers(63944, 524352, 0, 0, 0)
> [1606837.926] wl_keyboard@19.leave(63945, wl_surface@15)
> [1606837.945] wl_pointer@18.leave(63946, wl_surface@15)
> [1606837.956] wl_pointer@18.frame()
> [1606837.961] zwp_confined_pointer_v1@26.unconfined()
> Segmentation fault (core dumped)
> 
> To fix this, get the input from the window instead of the other way around.
> 
> Signed-off-by: Dima Ryazanov <dima@gmail.com>
> ---
>  clients/window.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 

Hi Dima,

thank you for the patch, and sorry, I know you have some very old
patches still waiting for reviews.

> diff --git a/clients/window.c b/clients/window.c
> index bcf2b017..dee4455f 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -286,6 +286,7 @@ struct window {
>  	confined_pointer_unconfined_handler_t pointer_unconfined_handler;
>  
>  	struct zwp_confined_pointer_v1 *confined_pointer;
> +	struct input *confined_input;
>  	struct widget *confined_widget;
>  	bool confined;
>  
> @@ -4788,8 +4789,8 @@ static void
>  locked_pointer_locked(void *data,
>  		      struct zwp_locked_pointer_v1 *locked_pointer)
>  {
> -	struct input *input = data;
> -	struct window *window = input->pointer_focus;
> +	struct window *window = data;
> +	struct input *input = window->locked_input;
>  
>  	window->pointer_locked = true;
>  
> @@ -4804,8 +4805,8 @@ static void
>  locked_pointer_unlocked(void *data,
>  			struct zwp_locked_pointer_v1 *locked_pointer)
>  {
> -	struct input *input = data;
> -	struct window *window = input->pointer_focus;
> +	struct window *window = data;
> +	struct input *input = window->locked_input;
>  
>  	window_unlock_pointer(window);
>  
> @@ -4860,7 +4861,7 @@ window_lock_pointer(struct window *window, struct input *input)
>  							ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_ONESHOT);
>  	zwp_locked_pointer_v1_add_listener(locked_pointer,
>  					   &locked_pointer_listener,
> -					   input);
> +					   window);

Now that this object will have a pointer to window, how do we safeguard
against window_destroy() getting called before we dispatch a locked or
unlocked event? Wouldn't that be necessary?

>  
>  	window->locked_input = input;
>  	window->locked_pointer = locked_pointer;
> @@ -4902,8 +4903,8 @@ static void
>  confined_pointer_confined(void *data,
>  			  struct zwp_confined_pointer_v1 *confined_pointer)
>  {
> -	struct input *input = data;
> -	struct window *window = input->pointer_focus;
> +	struct window *window = data;
> +	struct input *input = window->confined_input;
>  
>  	window->confined = true;
>  
> @@ -4918,8 +4919,8 @@ static void
>  confined_pointer_unconfined(void *data,
>  			    struct zwp_confined_pointer_v1 *confined_pointer)
>  {
> -	struct input *input = data;
> -	struct window *window = input->pointer_focus;
> +	struct window *window = data;
> +	struct input *input = window->confined_input;
>  
>  	window_unconfine_pointer(window);
>  
> @@ -4984,8 +4985,9 @@ window_confine_pointer_to_rectangles(struct window *window,
>  
>  	zwp_confined_pointer_v1_add_listener(confined_pointer,
>  					     &confined_pointer_listener,
> -					     input);
> +					     window);

The same safeguard question here.

>  
> +	window->confined_input = input;

I was also wondering about what would clean up this field when 'input'
gets destroyed, but I suppose a proper event sequence will clean things
up before 'input' is destroyed.

>  	window->confined_pointer = confined_pointer;
>  	window->confined_widget = NULL;
>  
> @@ -5046,6 +5048,7 @@ window_unconfine_pointer(struct window *window)
>  	zwp_confined_pointer_v1_destroy(window->confined_pointer);
>  	window->confined_pointer = NULL;
>  	window->confined = false;
> +	window->confined_input = NULL;
>  }
>  
>  static void

However, I do think this is better than not, so pushed:
   da188835..e0dc5d47  master -> master

I also filed a spec bug:
https://bugs.freedesktop.org/show_bug.cgi?id=106704


Thanks,
pq
On Tue, May 29, 2018 at 3:30 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Thu, 10 May 2018 00:53:38 -0700
> Dima Ryazanov <dima@gmail.com> wrote:
>
> > In GNOME (but not in Weston), if a window loses focus, the client first
> receives
> > the focus event, then the unlock/unconfine event. This causes toytoolkit
> to
> > dereference a NULL window when unlocking or unconfining the pointer.
> >
> > To repro:
> > - Run weston-confine
> > - Click the window
> > - Alt-Tab away from it
> >
> > Result:
> >
> > [1606837.869] wl_keyboard@19.modifiers(63944, 524352, 0, 0, 0)
> > [1606837.926] wl_keyboard@19.leave(63945, wl_surface@15)
> > [1606837.945] wl_pointer@18.leave(63946, wl_surface@15)
> > [1606837.956] wl_pointer@18.frame()
> > [1606837.961] zwp_confined_pointer_v1@26.unconfined()
> > Segmentation fault (core dumped)
> >
> > To fix this, get the input from the window instead of the other way
> around.
> >
> > Signed-off-by: Dima Ryazanov <dima@gmail.com>
> > ---
> >  clients/window.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> >
>
> Hi Dima,
>
> thank you for the patch, and sorry, I know you have some very old
> patches still waiting for reviews.
>

Haha, no worries!


> > diff --git a/clients/window.c b/clients/window.c
> > index bcf2b017..dee4455f 100644
> > --- a/clients/window.c
> > +++ b/clients/window.c
> > @@ -286,6 +286,7 @@ struct window {
> >       confined_pointer_unconfined_handler_t pointer_unconfined_handler;
> >
> >       struct zwp_confined_pointer_v1 *confined_pointer;
> > +     struct input *confined_input;
> >       struct widget *confined_widget;
> >       bool confined;
> >
> > @@ -4788,8 +4789,8 @@ static void
> >  locked_pointer_locked(void *data,
> >                     struct zwp_locked_pointer_v1 *locked_pointer)
> >  {
> > -     struct input *input = data;
> > -     struct window *window = input->pointer_focus;
> > +     struct window *window = data;
> > +     struct input *input = window->locked_input;
> >
> >       window->pointer_locked = true;
> >
> > @@ -4804,8 +4805,8 @@ static void
> >  locked_pointer_unlocked(void *data,
> >                       struct zwp_locked_pointer_v1 *locked_pointer)
> >  {
> > -     struct input *input = data;
> > -     struct window *window = input->pointer_focus;
> > +     struct window *window = data;
> > +     struct input *input = window->locked_input;
> >
> >       window_unlock_pointer(window);
> >
> > @@ -4860,7 +4861,7 @@ window_lock_pointer(struct window *window, struct
> input *input)
> >
>  ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_ONESHOT);
> >       zwp_locked_pointer_v1_add_listener(locked_pointer,
> >                                          &locked_pointer_listener,
> > -                                        input);
> > +                                        window);
>
> Now that this object will have a pointer to window, how do we safeguard
> against window_destroy() getting called before we dispatch a locked or
> unlocked event? Wouldn't that be necessary?
>

Ah, interesting, I did not think about that. I'll see if I can make it
happen. I'm guessing window_destroy should call window_unconfine_pointer and
window_unlock_pointer.

>
> >       window->locked_input = input;
> >       window->locked_pointer = locked_pointer;
> > @@ -4902,8 +4903,8 @@ static void
> >  confined_pointer_confined(void *data,
> >                         struct zwp_confined_pointer_v1 *confined_pointer)
> >  {
> > -     struct input *input = data;
> > -     struct window *window = input->pointer_focus;
> > +     struct window *window = data;
> > +     struct input *input = window->confined_input;
> >
> >       window->confined = true;
> >
> > @@ -4918,8 +4919,8 @@ static void
> >  confined_pointer_unconfined(void *data,
> >                           struct zwp_confined_pointer_v1
> *confined_pointer)
> >  {
> > -     struct input *input = data;
> > -     struct window *window = input->pointer_focus;
> > +     struct window *window = data;
> > +     struct input *input = window->confined_input;
> >
> >       window_unconfine_pointer(window);
> >
> > @@ -4984,8 +4985,9 @@ window_confine_pointer_to_rectangles(struct window
> *window,
> >
> >       zwp_confined_pointer_v1_add_listener(confined_pointer,
> >                                            &confined_pointer_listener,
> > -                                          input);
> > +                                          window);
>
> The same safeguard question here.
>
> >
> > +     window->confined_input = input;
>
> I was also wondering about what would clean up this field when 'input'
> gets destroyed, but I suppose a proper event sequence will clean things
> up before 'input' is destroyed.
>

Interesting. I can't convince myself that cleanup will happen correctly.
Now that I think about it, I wonder if I should've kept "input" in event
handlers - but instead of accessing input->pointer_focus, I should've added
new variables (confined_window, locked_window?).

Anyways, I'll try a few things. Still trying to figure out how all of this
works.

>       window->confined_pointer = confined_pointer;
> >       window->confined_widget = NULL;
> >
> > @@ -5046,6 +5048,7 @@ window_unconfine_pointer(struct window *window)
> >       zwp_confined_pointer_v1_destroy(window->confined_pointer);
> >       window->confined_pointer = NULL;
> >       window->confined = false;
> > +     window->confined_input = NULL;
> >  }
> >
> >  static void
>
> However, I do think this is better than not, so pushed:
>    da188835..e0dc5d47  master -> master
>
> I also filed a spec bug:
> https://bugs.freedesktop.org/show_bug.cgi?id=106704
>
>
> Thanks,
> pq
>
On Thu, 31 May 2018 00:16:14 -0700
Dima Ryazanov <dima@gmail.com> wrote:

> On Tue, May 29, 2018 at 3:30 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> 
> > On Thu, 10 May 2018 00:53:38 -0700
> > Dima Ryazanov <dima@gmail.com> wrote:
> >  
> > > In GNOME (but not in Weston), if a window loses focus, the client first  
> > receives  
> > > the focus event, then the unlock/unconfine event. This causes toytoolkit  
> > to  
> > > dereference a NULL window when unlocking or unconfining the pointer.
> > >
> > > To repro:
> > > - Run weston-confine
> > > - Click the window
> > > - Alt-Tab away from it
> > >
> > > Result:
> > >
> > > [1606837.869] wl_keyboard@19.modifiers(63944, 524352, 0, 0, 0)
> > > [1606837.926] wl_keyboard@19.leave(63945, wl_surface@15)
> > > [1606837.945] wl_pointer@18.leave(63946, wl_surface@15)
> > > [1606837.956] wl_pointer@18.frame()
> > > [1606837.961] zwp_confined_pointer_v1@26.unconfined()
> > > Segmentation fault (core dumped)
> > >
> > > To fix this, get the input from the window instead of the other way  
> > around.  
> > >
> > > Signed-off-by: Dima Ryazanov <dima@gmail.com>
> > > ---
> > >  clients/window.c | 23 +++++++++++++----------
> > >  1 file changed, 13 insertions(+), 10 deletions(-)

> > > @@ -4860,7 +4861,7 @@ window_lock_pointer(struct window *window, struct  
> > input *input)  
> > >  
> >  ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_ONESHOT);  
> > >       zwp_locked_pointer_v1_add_listener(locked_pointer,
> > >                                          &locked_pointer_listener,
> > > -                                        input);
> > > +                                        window);  
> >
> > Now that this object will have a pointer to window, how do we safeguard
> > against window_destroy() getting called before we dispatch a locked or
> > unlocked event? Wouldn't that be necessary?
> >  
> 
> Ah, interesting, I did not think about that. I'll see if I can make it
> happen. I'm guessing window_destroy should call window_unconfine_pointer and
> window_unlock_pointer.

> > > @@ -4984,8 +4985,9 @@ window_confine_pointer_to_rectangles(struct window  
> > *window,  
> > >
> > >       zwp_confined_pointer_v1_add_listener(confined_pointer,
> > >                                            &confined_pointer_listener,
> > > -                                          input);
> > > +                                          window);  
> >
> > The same safeguard question here.
> >  
> > >
> > > +     window->confined_input = input;  
> >
> > I was also wondering about what would clean up this field when 'input'
> > gets destroyed, but I suppose a proper event sequence will clean things
> > up before 'input' is destroyed.
> >  
> 
> Interesting. I can't convince myself that cleanup will happen correctly.
> Now that I think about it, I wonder if I should've kept "input" in event
> handlers - but instead of accessing input->pointer_focus, I should've added
> new variables (confined_window, locked_window?).
> 
> Anyways, I'll try a few things. Still trying to figure out how all of this
> works.

Good luck! I appreciate the effort and I would be exactly in the same
place, not having much clue about how window.c works today. :-)


Thanks,
pq