| Message ID | 20180905084927.17339-1-ofourdan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
"xwayland: Clean-up present after destroying common bits"
( rev:
4
)
in
X.org (DEPRECATED - USE GITLAB) |
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); }
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); > } >
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
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.
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(+)