Re: [PATCH xserver v2] xwayland: Remove xwl_present_window from privates on cleanup

2018-09-05 Thread Michel Dänzer
On 2018-09-05 10:35 a.m., Olivier Fourdan wrote:
> Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()`
> before the common `DestroyWindow()`.
> 
> But then `DestroyWindow()` calls `present_destroy_window()` which will
> possibly 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)
> 
> This is because `xwl_present_cleanup()` frees the memory but does not
> remove it from the window's privates, and `xwl_present_abort_vblank()`
> will still find it and hence try to access that freed memory...
> 
> Clear `xwl_present_cleanup()` after `DestroyWindow()` so that
> `xwl_present_abort_vblank()` can still access valid memory before it's
> freed.

This last paragraph doesn't seem to match the rest of the patch.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
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

[PATCH xserver v2] xwayland: Remove xwl_present_window from privates on cleanup

2018-09-05 Thread Olivier Fourdan
Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()`
before the common `DestroyWindow()`.

But then `DestroyWindow()` calls `present_destroy_window()` which will
possibly 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)

This is because `xwl_present_cleanup()` frees the memory but does not
remove it from the window's privates, and `xwl_present_abort_vblank()`
will still find it and hence try to access that freed memory...

Clear `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 
---
 v2: Remove leftovers from broken initial patch...

 hw/xwayland/xwayland-present.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 81e0eb9ce..316e04443 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -147,6 +147,11 @@ xwl_present_cleanup(WindowPtr window)
 /* Clear timer */
 xwl_present_free_timer(xwl_present_window);
 
+/* Remove from privates so we don't try to access it later */
+dixSetPrivate(>devPrivates,
+  _present_window_private_key,
+  NULL);
+
 free(xwl_present_window);
 }
 
-- 
2.17.1

___
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