[xserver,v3] xwayland: Remove xwl_present_window from privates on cleanup

Submitted by Olivier Fourdan on Sept. 5, 2018, 8:49 a.m.

Details

Message ID 20180905084927.17339-1-ofourdan@redhat.com
State New
Series "xwayland: Clean-up present after destroying common bits"
Headers show

Commit Message

Olivier Fourdan Sept. 5, 2018, 8:49 a.m.
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...

Remove `xwl_present_window` from window's privates on cleanup so that no
other function can find and reuse that data once it's freed.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
---
 v2: Remove leftovers from broken initial patch...
 v3: Rephrase non-sensical explanation of the fix

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

Patch hide | download patch | download mbox

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(&window->devPrivates,
+                  &xwl_present_window_private_key,
+                  NULL);
+
     free(xwl_present_window);
 }
 

Comments

Lionel Landwerlin Sept. 5, 2018, 10:05 a.m.
On 05/09/2018 09:49, 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...
>
> Remove `xwl_present_window` from window's privates on cleanup so that no
> other function can find and reuse that data once it's freed.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269


We also have this fdo bug : 
https://bugs.freedesktop.org/show_bug.cgi?id=107314


> Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
> ---
>   v2: Remove leftovers from broken initial patch...
>   v3: Rephrase non-sensical explanation of the fix
>
>   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(&window->devPrivates,
> +                  &xwl_present_window_private_key,
> +                  NULL);
> +
>       free(xwl_present_window);
>   }
>
Olivier Fourdan Sept. 5, 2018, 10:44 a.m.
Hi,

On Wed, Sep 5, 2018 at 12:05 PM Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
>
> On 05/09/2018 09:49, Olivier Fourdan wrote:
> > [...]
> > 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...
> >
> > Remove `xwl_present_window` from window's privates on cleanup so that no
> > other function can find and reuse that data once it's freed.
> >
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269
>
>
> We also have this fdo bug :
> https://bugs.freedesktop.org/show_bug.cgi?id=107314

Yeap, I know about this bug and the patch you sent, but the backtrace
is different, it doesn't occur on DestroyWindow(), so I doubt this is
the same (unless you tried my fix and it fixes your issue as well...)

Cheers,
Olivier
Lionel Landwerlin Sept. 5, 2018, 11:04 a.m.
On 05/09/2018 11:44, Olivier Fourdan wrote:
> Hi,
>
> On Wed, Sep 5, 2018 at 12:05 PM Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
>> On 05/09/2018 09:49, Olivier Fourdan wrote:
>>> [...]
>>> 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...
>>>
>>> Remove `xwl_present_window` from window's privates on cleanup so that no
>>> other function can find and reuse that data once it's freed.
>>>
>>> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269
>>
>> We also have this fdo bug :
>> https://bugs.freedesktop.org/show_bug.cgi?id=107314
> Yeap, I know about this bug and the patch you sent, but the backtrace
> is different, it doesn't occur on DestroyWindow(), so I doubt this is
> the same (unless you tried my fix and it fixes your issue as well...)
>
> Cheers,
> Olivier
> _______________________________________________


Oops indeed.