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

[Dima Ryazanov]

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

> On Thu, 10 May 2018 00:53:38 -0700
> Dima Ryazanov <dima at 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 at 19.modifiers(63944, 524352, 0, 0, 0)
> > [1606837.926] wl_keyboard at 19.leave(63945, wl_surface at 15)
> > [1606837.945] wl_pointer at 18.leave(63946, wl_surface at 15)
> > [1606837.956] wl_pointer at 18.frame()
> > [1606837.961] zwp_confined_pointer_v1 at 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 at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180531/c9f1fd45/attachment.html>