Hi Roman, (sorry for top posting, that's to keep the patch after the message)
So, with that patch I just sent, there is no more access-after-free in unrealize() but as I said intially, ther eiare still acess-after-free in the pending buffers/events, i.e.: Invalid read of size 8 at 0x4348A3: xwl_present_buffer_release (xwayland-present.c:134) by 0x823203D: ffi_call_unix64 (unix64.S:76) by 0x82319FE: ffi_call (ffi64.c:525) by 0x56C32DC: wl_closure_invoke (connection.c:996) by 0x56BFA38: dispatch_event.isra.6 (wayland-client.c:1434) by 0x56C0F5B: dispatch_queue (wayland-client.c:1580) by 0x56C0F5B: wl_display_dispatch_queue_pending (wayland-client.c:1822) by 0x42A83A: xwl_read_events (xwayland.c:800) by 0x58B5D0: ospoll_wait (ospoll.c:651) by 0x584E6B: WaitForSomething (WaitFor.c:208) by 0x5549AF: Dispatch (dispatch.c:421) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) Address 0xfcab338 is 104 bytes inside a block of size 184 free'd at 0x4C2EDAC: free (vg_replace_malloc.c:530) by 0x42BB78: xwl_unrealize_window (xwayland.c:626) by 0x541F69: compUnrealizeWindow (compwindow.c:285) by 0x57E27A: UnrealizeTree (window.c:2816) by 0x581209: UnmapWindow (window.c:2874) by 0x54EBA6: ProcUnmapWindow (dispatch.c:879) by 0x554BFD: Dispatch (dispatch.c:479) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) Block was alloc'd at at 0x4C2FB06: calloc (vg_replace_malloc.c:711) by 0x42B389: xwl_realize_window (xwayland.c:486) by 0x541ED9: compRealizeWindow (compwindow.c:268) by 0x57DAC0: RealizeTree (window.c:2617) by 0x580BA8: MapWindow (window.c:2694) by 0x54EAAA: ProcMapWindow (dispatch.c:845) by 0x554BFD: Dispatch (dispatch.c:479) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) and: at 0x434882: __xorg_list_del (list.h:183) by 0x434882: xorg_list_del (list.h:204) by 0x434882: xwl_present_free_event (xwayland-present.c:112) by 0x434882: xwl_present_buffer_release (xwayland-present.c:138) by 0x823203D: ffi_call_unix64 (unix64.S:76) by 0x82319FE: ffi_call (ffi64.c:525) by 0x56C32DC: wl_closure_invoke (connection.c:996) by 0x56BFA38: dispatch_event.isra.6 (wayland-client.c:1434) by 0x56C0F5B: dispatch_queue (wayland-client.c:1580) by 0x56C0F5B: wl_display_dispatch_queue_pending (wayland-client.c:1822) by 0x42A83A: xwl_read_events (xwayland.c:800) by 0x58B5D0: ospoll_wait (ospoll.c:651) by 0x584E6B: WaitForSomething (WaitFor.c:208) by 0x5549AF: Dispatch (dispatch.c:421) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) Address 0xfcab380 is 176 bytes inside a block of size 184 free'd at 0x4C2EDAC: free (vg_replace_malloc.c:530) by 0x42BB78: xwl_unrealize_window (xwayland.c:626) by 0x541F69: compUnrealizeWindow (compwindow.c:285) by 0x57E27A: UnrealizeTree (window.c:2816) by 0x581209: UnmapWindow (window.c:2874) by 0x54EBA6: ProcUnmapWindow (dispatch.c:879) by 0x554BFD: Dispatch (dispatch.c:479) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) Block was alloc'd at at 0x4C2FB06: calloc (vg_replace_malloc.c:711) by 0x42B389: xwl_realize_window (xwayland.c:486) by 0x541ED9: compRealizeWindow (compwindow.c:268) by 0x57DAC0: RealizeTree (window.c:2617) by 0x580BA8: MapWindow (window.c:2694) by 0x54EAAA: ProcMapWindow (dispatch.c:845) by 0x554BFD: Dispatch (dispatch.c:479) by 0x558C65: dix_main (main.c:276) by 0x79F81BA: (below main) (libc-start.c:308) That's what I meant by “elaborate from there” in my previous post :) Cheers, Olivier On Wed, Apr 18, 2018 at 3:29 PM, Olivier Fourdan <ofour...@redhat.com> wrote: > xwl_unrealize_window() would use freed xwl_window which can lead to > various memory corruption and crashes, as reported by valgrind: > > Invalid read of size 8 > at 0x42C802: xwl_present_cleanup (xwayland-present.c:84) > by 0x42BA67: xwl_unrealize_window (xwayland.c:601) > by 0x541EE9: compUnrealizeWindow (compwindow.c:285) > by 0x57E1FA: UnrealizeTree (window.c:2816) > by 0x581189: UnmapWindow (window.c:2874) > by 0x54EB26: ProcUnmapWindow (dispatch.c:879) > by 0x554B7D: Dispatch (dispatch.c:479) > by 0x558BE5: dix_main (main.c:276) > by 0x7C4B1BA: (below main) (libc-start.c:308) > Address 0xf520f60 is 96 bytes inside a block of size 184 free'd > at 0x4C2EDAC: free (vg_replace_malloc.c:530) > by 0x42B9FB: xwl_unrealize_window (xwayland.c:624) > by 0x541EE9: compUnrealizeWindow (compwindow.c:285) > by 0x57E1FA: UnrealizeTree (window.c:2816) > by 0x581189: UnmapWindow (window.c:2874) > by 0x54EB26: ProcUnmapWindow (dispatch.c:879) > by 0x554B7D: Dispatch (dispatch.c:479) > by 0x558BE5: dix_main (main.c:276) > by 0x7C4B1BA: (below main) (libc-start.c:308) > Block was alloc'd at > at 0x4C2FB06: calloc (vg_replace_malloc.c:711) > by 0x42B307: xwl_realize_window (xwayland.c:488) > by 0x541E59: compRealizeWindow (compwindow.c:268) > by 0x57DA40: RealizeTree (window.c:2617) > by 0x580B28: MapWindow (window.c:2694) > by 0x54EA2A: ProcMapWindow (dispatch.c:845) > by 0x554B7D: Dispatch (dispatch.c:479) > by 0x558BE5: dix_main (main.c:276) > by 0x7C4B1BA: (below main) (libc-start.c:308) > > This is because UnrealizeTree() traverses the tree from top to bottom, > which invalidates the assumption that if the Window doesn't feature an > xwl_window on its own, it's the xwl_window of its first ancestor with > one. > > This partially revert commit 82df2ce3 > > Signed-off-by: Olivier Fourdan <ofour...@redhat.com> > --- > hw/xwayland/xwayland-input.c | 2 +- > hw/xwayland/xwayland-present.c | 22 +++++-------- > hw/xwayland/xwayland.c | 60 ++++++++++++++++------------------ > hw/xwayland/xwayland.h | 4 +-- > 4 files changed, 40 insertions(+), 48 deletions(-) > > diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c > index 74a2579f7..0a37f97bd 100644 > --- a/hw/xwayland/xwayland-input.c > +++ b/hw/xwayland/xwayland-input.c > @@ -1067,7 +1067,7 @@ xwl_keyboard_activate_grab(DeviceIntPtr device, > GrabPtr grab, TimeStamp time, Bo > if (xwl_seat == NULL) > xwl_seat = find_matching_seat(device); > if (xwl_seat) > - set_grab(xwl_seat, xwl_window_of_top(grab->window)); > + set_grab(xwl_seat, xwl_window_from_window(grab->window)); > } > > ActivateKeyboardGrab(device, grab, time, passive); > diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland- > present.c > index f403ff701..62c70a820 100644 > --- a/hw/xwayland/xwayland-present.c > +++ b/hw/xwayland/xwayland-present.c > @@ -73,13 +73,9 @@ xwl_present_reset_timer(struct xwl_window *xwl_window) > } > > void > -xwl_present_cleanup(WindowPtr window) > +xwl_present_cleanup(struct xwl_window *xwl_window, WindowPtr window) > { > - struct xwl_window *xwl_window = xwl_window_of_top(window); > - struct xwl_present_event *event, *tmp; > - > - if (!xwl_window) > - return; > + struct xwl_present_event *event, *tmp; > > if (xwl_window->present_window == window) { > if (xwl_window->present_frame_callback) { > @@ -258,7 +254,7 @@ static const struct wl_callback_listener > xwl_present_sync_listener = { > static RRCrtcPtr > xwl_present_get_crtc(WindowPtr present_window) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > if (xwl_window == NULL) > return NULL; > > @@ -268,7 +264,7 @@ xwl_present_get_crtc(WindowPtr present_window) > static int > xwl_present_get_ust_msc(WindowPtr present_window, uint64_t *ust, > uint64_t *msc) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > if (!xwl_window) > return BadAlloc; > *ust = xwl_window->present_ust; > @@ -297,7 +293,7 @@ xwl_present_queue_vblank(WindowPtr present_window, > uint64_t event_id, > uint64_t msc) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > struct xwl_present_event *event; > > if (!xwl_window) > @@ -337,7 +333,7 @@ xwl_present_abort_vblank(WindowPtr present_window, > uint64_t event_id, > uint64_t msc) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > struct xwl_present_event *event, *tmp; > > if (!xwl_window) > @@ -374,7 +370,7 @@ xwl_present_check_flip2(RRCrtcPtr crtc, > Bool sync_flip, > PresentFlipReason *reason) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > > if (!xwl_window) > return FALSE; > @@ -415,7 +411,7 @@ xwl_present_flip(WindowPtr present_window, > Bool sync_flip, > RegionPtr damage) > { > - struct xwl_window *xwl_window = xwl_window_of_top(present_ > window); > + struct xwl_window *xwl_window = xwl_window_from_window( > present_window); > BoxPtr present_box, damage_box; > Bool buffer_created; > struct wl_buffer *buffer; > @@ -485,7 +481,7 @@ xwl_present_flip(WindowPtr present_window, > static void > xwl_present_flips_stop(WindowPtr window) > { > - struct xwl_window *xwl_window = xwl_window_of_top(window); > + struct xwl_window *xwl_window = xwl_window_from_window(window); > > if (!xwl_window) > return; > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c > index 44bbc3b18..72493285e 100644 > --- a/hw/xwayland/xwayland.c > +++ b/hw/xwayland/xwayland.c > @@ -122,21 +122,10 @@ static DevPrivateKeyRec xwl_window_private_key; > static DevPrivateKeyRec xwl_screen_private_key; > static DevPrivateKeyRec xwl_pixmap_private_key; > > -struct xwl_window * > -xwl_window_of_top(WindowPtr window) > -{ > - return dixLookupPrivate(&window->devPrivates, > &xwl_window_private_key); > -} > - > static struct xwl_window * > -xwl_window_of_self(WindowPtr window) > +xwl_window_get(WindowPtr window) > { > - struct xwl_window *xwl_window = dixLookupPrivate(&window->devPrivates, > &xwl_window_private_key); > - > - if (xwl_window && xwl_window->window == window) > - return xwl_window; > - else > - return NULL; > + return dixLookupPrivate(&window->devPrivates, > &xwl_window_private_key); > } > > struct xwl_screen * > @@ -223,7 +212,7 @@ xwl_property_callback(CallbackListPtr *pcbl, void > *closure, > if (rec->win->drawable.pScreen != screen) > return; > > - xwl_window = xwl_window_of_self(rec->win); > + xwl_window = xwl_window_get(rec->win); > if (!xwl_window) > return; > > @@ -262,6 +251,22 @@ xwl_close_screen(ScreenPtr screen) > return screen->CloseScreen(screen); > } > > +struct xwl_window * > +xwl_window_from_window(WindowPtr window) > +{ > + struct xwl_window *xwl_window; > + > + while (window) { > + xwl_window = xwl_window_get(window); > + if (xwl_window) > + return xwl_window; > + > + window = window->parent; > + } > + > + return NULL; > +} > + > static struct xwl_seat * > xwl_screen_get_default_seat(struct xwl_screen *xwl_screen) > { > @@ -292,7 +297,7 @@ xwl_cursor_warped_to(DeviceIntPtr device, > if (!window) > window = XYToWindow(sprite, x, y); > > - xwl_window = xwl_window_of_top(window); > + xwl_window = xwl_window_from_window(window); > if (!xwl_window && xwl_seat->focus_window) { > focus = xwl_seat->focus_window->window; > > @@ -339,7 +344,7 @@ xwl_cursor_confined_to(DeviceIntPtr device, > return; > } > > - xwl_window = xwl_window_of_top(window); > + xwl_window = xwl_window_from_window(window); > if (!xwl_window && xwl_seat->focus_window) { > /* Allow confining on InputOnly windows, but only if the geometry > * is the same than the focus window. > @@ -455,7 +460,6 @@ xwl_realize_window(WindowPtr window) > struct xwl_screen *xwl_screen; > struct xwl_window *xwl_window; > struct wl_region *region; > - Bool create_xwl_window = TRUE; > Bool ret; > > xwl_screen = xwl_screen_get(screen); > @@ -475,17 +479,11 @@ xwl_realize_window(WindowPtr window) > > if (xwl_screen->rootless) { > if (window->redirectDraw != RedirectDrawManual) > - create_xwl_window = FALSE; > + return ret; > } > else { > if (window->parent) > - create_xwl_window = FALSE; > - } > - > - if (!create_xwl_window) { > - if (window->parent) > - dixSetPrivate(&window->devPrivates, &xwl_window_private_key, > xwl_window_of_top(window->parent)); > - return ret; > + return ret; > } > > xwl_window = calloc(1, sizeof *xwl_window); > @@ -602,9 +600,9 @@ xwl_unrealize_window(WindowPtr window) > compUnredirectWindow(serverClient, window, CompositeRedirectManual); > > #ifdef GLAMOR_HAS_GBM > - if (xwl_screen->present) > - /* Always cleanup Present (Present might have been active on > child window) */ > - xwl_present_cleanup(window); > + xwl_window = xwl_window_from_window(window); > + if (xwl_window && xwl_screen->present) > + xwl_present_cleanup(xwl_window, window); > #endif > > screen->UnrealizeWindow = xwl_screen->UnrealizeWindow; > @@ -612,11 +610,9 @@ xwl_unrealize_window(WindowPtr window) > xwl_screen->UnrealizeWindow = screen->UnrealizeWindow; > screen->UnrealizeWindow = xwl_unrealize_window; > > - xwl_window = xwl_window_of_self(window); > - if (!xwl_window) { > - dixSetPrivate(&window->devPrivates, &xwl_window_private_key, > NULL); > + xwl_window = xwl_window_get(window); > + if (!xwl_window) > return ret; > - } > > wl_surface_destroy(xwl_window->surface); > xorg_list_del(&xwl_window->link_damage); > diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h > index cf2551b99..11a9f4816 100644 > --- a/hw/xwayland/xwayland.h > +++ b/hw/xwayland/xwayland.h > @@ -361,7 +361,7 @@ RRModePtr xwayland_cvt(int HDisplay, int VDisplay, > void xwl_pixmap_set_private(PixmapPtr pixmap, struct xwl_pixmap > *xwl_pixmap); > struct xwl_pixmap *xwl_pixmap_get(PixmapPtr pixmap); > > -struct xwl_window *xwl_window_of_top(WindowPtr window); > +struct xwl_window *xwl_window_from_window(WindowPtr window); > > Bool xwl_shm_create_screen_resources(ScreenPtr screen); > PixmapPtr xwl_shm_create_pixmap(ScreenPtr screen, int width, int height, > @@ -383,7 +383,7 @@ struct wl_buffer > *xwl_glamor_pixmap_get_wl_buffer(PixmapPtr > pixmap, > > #ifdef GLAMOR_HAS_GBM > Bool xwl_present_init(ScreenPtr screen); > -void xwl_present_cleanup(WindowPtr window); > +void xwl_present_cleanup(struct xwl_window *xwl_window, WindowPtr window); > #endif > > void xwl_screen_release_tablet_manager(struct xwl_screen *xwl_screen); > -- > 2.17.0 > >
_______________________________________________ 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