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

Reply via email to