[PATCH xserver] xwayland: Clean-up present after destroying common bits

[Olivier Fourdan]

On Wed, Sep 5, 2018 at 9:48 AM Olivier Fourdan <ofourdan at redhat.com> wrote:
>
> Right now, `xwl_destroy_window()` invokes `xwl_present_cleanup()` before
> the common `DestroyWindow()`.
>
> But `DestroyWindow()` calls in `present_destroy_window()` which will
> then end up in `xwl_present_abort_vblank()` which will try to access
> data that was previously freed by `xwl_present_cleanup()`:
>
>   Invalid read of size 8
>      at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378)
>      by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651)
>      by 0x53695A: present_free_window_vblank (present_screen.c:87)
>      by 0x53695A: present_destroy_window (present_screen.c:152)
>      by 0x42A90D: xwl_destroy_window (xwayland.c:653)
>      by 0x584298: compDestroyWindow (compwindow.c:613)
>      by 0x53CEE3: damageDestroyWindow (damage.c:1570)
>      by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326)
>      by 0x46F7F6: FreeWindowResources (window.c:1031)
>      by 0x472847: DeleteWindow (window.c:1099)
>      by 0x46B54C: doFreeResource (resource.c:880)
>      by 0x46C706: FreeClientResources (resource.c:1146)
>      by 0x446ADE: CloseDownClient (dispatch.c:3473)
>    Address 0x182abde0 is 80 bytes inside a block of size 112 free'd
>      at 0x4C2FDAC: free (vg_replace_malloc.c:530)
>      by 0x42A937: xwl_destroy_window (xwayland.c:647)
>      by 0x584298: compDestroyWindow (compwindow.c:613)
>      by 0x53CEE3: damageDestroyWindow (damage.c:1570)
>      by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326)
>      by 0x46F7F6: FreeWindowResources (window.c:1031)
>      by 0x472847: DeleteWindow (window.c:1099)
>      by 0x46B54C: doFreeResource (resource.c:880)
>      by 0x46C706: FreeClientResources (resource.c:1146)
>      by 0x446ADE: CloseDownClient (dispatch.c:3473)
>      by 0x446DA5: ProcKillClient (dispatch.c:3279)
>      by 0x4476AF: Dispatch (dispatch.c:479)
>    Block was alloc'd at
>      at 0x4C30B06: calloc (vg_replace_malloc.c:711)
>      by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54)
>      by 0x434228: xwl_present_get_crtc (xwayland-present.c:302)
>      by 0x539728: proc_present_query_capabilities (present_request.c:227)
>      by 0x4476AF: Dispatch (dispatch.c:479)
>      by 0x44B5B5: dix_main (main.c:276)
>      by 0x75F611A: (below main) (libc-start.c:308)
>
> Move `xwl_present_cleanup()` after `DestroyWindow()` so that
> `xwl_present_abort_vblank()` can still access valid memory before it's
> freed.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> ---
>  hw/xwayland/xwayland.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> index 96b4db18c..63d69fb3a 100644
> --- a/hw/xwayland/xwayland.c
> +++ b/hw/xwayland/xwayland.c
> @@ -642,11 +642,6 @@ xwl_destroy_window(WindowPtr window)
>      struct xwl_screen *xwl_screen = xwl_screen_get(screen);
>      Bool ret;
>
> -#ifdef GLAMOR_HAS_GBM
> -    if (xwl_screen->present)
> -        xwl_present_cleanup(window);
> -#endif
> -
>      screen->DestroyWindow = xwl_screen->DestroyWindow;
>
>      if (screen->DestroyWindow)
> @@ -657,6 +652,11 @@ xwl_destroy_window(WindowPtr window)
>      xwl_screen->DestroyWindow = screen->DestroyWindow;
>      screen->DestroyWindow = xwl_destroy_window;
>
> +#ifdef GLAMOR_HAS_GBM
> +    if (xwl_screen->present)
> +        xwl_present_cleanup(window);
> +#endif
> +
>      return ret;
>  }
>
> --
> 2.17.1
>

Sorry, not enough coffee, I take that back, obviously the crash won't
occur because `xwl_present_cleanup()` will fail to match the window
once the resources have been freed so we return early in
`xwl_present_cleanup()`...

Will try again.

Cheers,
Olivier