[PATCH xserver] xwayland: Clear up x_cursor on UnrealizeCursor()

[Jonas Ådahl]

On Thu, Sep 22, 2016 at 08:34:59AM +0200, Olivier Fourdan wrote:
> In Xwayland's xwl_unrealize_cursor(), the x_cursor is cleared up only
> when a device value is provided to the UnrealizeCursor() routine, but
> if the device is NULL as called from FreeCursor(), the corresponding
> x_cursor for the xwl_seat is left untouched.
> 
> This might cause a segfault when trying to access the unrealized
> cursor's devPrivates in xwl_seat_set_cursor().
> 
> A possible occurrence of this is the client changing the cursor, the
> Xserver calling FreeCursor() which does UnrealizeCursor() and then
> the Wayland server sending a pointer enter event, which invokes
> xwl_seat_set_cursor() while the seat's x_cursor has just been
> unrealized.
> 
> To avoid this, walk through all the xwl_seats and clear up all x_cursor
> matching the cursor being unrealized.
> 
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> ---
>  hw/xwayland/xwayland-cursor.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
> index 7d14a3d..0f22b5d 100644
> --- a/hw/xwayland/xwayland-cursor.c
> +++ b/hw/xwayland/xwayland-cursor.c
> @@ -76,7 +76,8 @@ static Bool
>  xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen, CursorPtr cursor)
>  {
>      PixmapPtr pixmap;
> -    struct xwl_seat *xwl_seat;
> +    struct xwl_screen *xwl_screen;
> +    struct xwl_seat *xwl_seat, *next_xwl_seat;
>  
>      pixmap = dixGetPrivate(&cursor->devPrivates, &xwl_cursor_private_key);
>      if (!pixmap)
> @@ -85,9 +86,10 @@ xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen, CursorPtr cursor)
>      dixSetPrivate(&cursor->devPrivates, &xwl_cursor_private_key, NULL);
>  
>      /* When called from FreeCursor(), device is always NULL */
> -    if (device) {
> -        xwl_seat = device->public.devicePrivate;
> -        if (xwl_seat && cursor == xwl_seat->x_cursor)
> +    xwl_screen = xwl_screen_get(screen);
> +    xorg_list_for_each_entry_safe(xwl_seat, next_xwl_seat,
> +                                  &xwl_screen->seat_list, link) {

No need to use _safe() here. We only change a field of the xwl_seat, we
never unlink it.

Other than that, this is

Reviewed-by: Jonas Ådahl <jadahl at gmail.com>


Jonas

> +        if (cursor == xwl_seat->x_cursor)
>              xwl_seat->x_cursor = NULL;
>      }
>  
> -- 
> 2.9.3
>