Re: [PATCH xserver v3] xwayland: Fix use after free of cursors

[Olivier Fourdan]

Hi,

> > Sometimes, Xwayland will try to use a cursor that has just been freed,
> > leading to a crash when trying to access that cursor data either in
> > miPointerUpdateSprite() or AnimCurTimerNotify().
> > 
> > CheckMotion() updates the pointer's cursor based on which xwindow
> > XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
> > to fake a crossing to the root window when the pointer has left the
> > Wayland surface but is still within the xwindow.
> > 
> > But after an xwindow is unrealized, the last xwindow used to match the
> > xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
> > not return the same xwindow.
> > 
> > To avoid this issue, update the last_xwindow based on enter and leave
> > notifications instead of xwl_xy_to_window(), and check if the xwindow
> > found by the regular miXYToWindow() is a child of the known last
> > xwindow, so that multiple consecutive calls to xwl_xy_to_window()
> > return the same xwindow, being either the one found by miXYToWindow()
> > or the root window.
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
> > Signed-off-by: Olivier Fourdan 
> 
> Tested-by: Vít Ondruch 
> Tested-by: Satish Balay 

I have added this patch to the Fedora 25 package for 
xorg-x11-server-Xwayland-1.19.0 a week or so ago and haven't spotted any new 
report of a similar crash in Xwayland since then, so I am quite confident this 
is the right fix for the issue.

Cheers,
Olivier.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v3] xwayland: Fix use after free of cursors

[Olivier Fourdan]

> Sometimes, Xwayland will try to use a cursor that has just been freed,
> leading to a crash when trying to access that cursor data either in
> miPointerUpdateSprite() or AnimCurTimerNotify().
> 
> CheckMotion() updates the pointer's cursor based on which xwindow
> XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
> to fake a crossing to the root window when the pointer has left the
> Wayland surface but is still within the xwindow.
> 
> But after an xwindow is unrealized, the last xwindow used to match the
> xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
> not return the same xwindow.
> 
> To avoid this issue, update the last_xwindow based on enter and leave
> notifications instead of xwl_xy_to_window(), and check if the xwindow
> found by the regular miXYToWindow() is a child of the known last
> xwindow, so that multiple consecutive calls to xwl_xy_to_window()
> return the same xwindow, being either the one found by miXYToWindow()
> or the root window.
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
> Signed-off-by: Olivier Fourdan 

Tested-by: Vít Ondruch 
Tested-by: Satish Balay 

> ---
>  v2: Issue was caused by inconsistent returned value from
>  xwl_xy_to_window() after a window is unrealized, fix the
>  issue to update the last_xwindow on enter/leave notify handlers.
>  Fix was succesfully tested in:
>  https://bugzilla.redhat.com/show_bug.cgi?id=1387281
>  v3: Same patch, reword the commit messag to make it shorter and clearer
>  (apparently patchwork did not like the long message much...)
> 
>  hw/xwayland/xwayland-input.c | 17 +
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 0526122..681bc9d 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer
> *pointer,
>  dx = xwl_seat->focus_window->window->drawable.x;
>  dy = xwl_seat->focus_window->window->drawable.y;
>  
> +/* We just entered a new xwindow, forget about the old last xwindow */
> +xwl_seat->last_xwindow = NullWindow;
> +
>  master = GetMaster(dev, POINTER_OR_FLOAT);
>  (*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
>  
> @@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer
> *pointer,
>  
>  xwl_seat->xwl_screen->serial = serial;
>  
> -xwl_seat->focus_window = NULL;
> -CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
> +/* The pointer has left a known xwindow, save it for a possible match
> + * in sprite_check_lost_focus()
> + */
> +if (xwl_seat->focus_window) {
> +xwl_seat->last_xwindow = xwl_seat->focus_window->window;
> +xwl_seat->focus_window = NULL;
> +CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
> +}
>  }
>  
>  static void
> @@ -1256,10 +1265,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr
> window)
>   */
>  if (master->lastSlave == xwl_seat->pointer &&
>  xwl_seat->focus_window == NULL &&
> -xwl_seat->last_xwindow == window)
> +xwl_seat->last_xwindow != NullWindow &&
> +IsParent (xwl_seat->last_xwindow, window))
>  return TRUE;
>  
> -xwl_seat->last_xwindow = window;
>  return FALSE;
>  }
>  
> --
> 2.9.3
> 
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel