[Mesa-dev] [PATCH] st/mesa: add destroy_drawable interface

Charmaine Lee charmainel at vmware.com
Sat Jul 22 01:05:47 UTC 2017


Hi Christoph,

Can you provide an apitrace of the test that crashes?

Thanks.
-Charmaine

________________________________________
From: mesa-dev <mesa-dev-bounces at lists.freedesktop.org> on behalf of Christoph Haag <haagch+mesadev at frickel.club>
Sent: Friday, July 21, 2017 4:52:41 PM
To: mesa-dev at lists.freedesktop.org
Subject: Re: [Mesa-dev] [PATCH] st/mesa: add destroy_drawable interface

This patch breaks steam for me. Segfault backtrace:

#0  0x00000023 in ?? ()
No symbol table info available.
#1  0xf6b1a417 in _mesa_hash_table_search (ht=0x585bb7e8, key=0x5820ee88) at hash_table.c:246
        __PRETTY_FUNCTION__ = "_mesa_hash_table_search"
#2  0xf6a4488e in st_framebuffer_iface_remove (stfbi=0x5820ee88) at state_tracker/st_manager.c:545
        entry = 0xf6bb3038 <swap_fences_unref+86>
#3  0xf6a448e5 in st_api_destroy_drawable (stapi=0xf7212bc0 <st_gl_api>, stfbi=0x5820ee88) at state_tracker/st_manager.c:564
No locals.
#4  0xf6bb2b64 in dri_destroy_buffer (dPriv=0x57d62560) at dri_drawable.c:186
        drawable = 0x5820ee88
        screen = 0x57e1a0a8
        stapi = 0xf7212bc0 <st_gl_api>
        i = 7
#5  0xf6bb132c in dri_put_drawable (pdp=0x57d62560) at dri_util.c:642
No locals.
#6  0xf6bb145c in driDestroyDrawable (pdp=0x57d62560) at dri_util.c:695
No locals.
#7  0xf759249a in loader_dri3_drawable_fini (draw=0x581fb0f0) at loader_dri3_helper.c:109
        i = 1478471608
#8  0xf758ca73 in dri3_destroy_drawable (base=0x581fb0d0) at dri3_glx.c:366
        pdraw = 0x581fb0d0
#9  0xf7583b5a in driReleaseDrawables (gc=0x57e19ef8) at dri_common.c:452
        priv = 0x57e082d0
        pdraw = 0x581fb0d0
#10 0xf758c679 in dri3_bind_context (context=0x57e19ef8, old=0x57e19ef8, draw=165675047, read=165675047) at dri3_glx.c:223
        pcp = 0x57e19ef8
        psc = 0x57e07608
        pdraw = 0x5866eb28
        pread = 0x5866eb28
        dri_draw = 0x0
        dri_read = 0x0
#11 0xf754cd12 in MakeContextCurrent (dpy=0x57c16f30, draw=165675047, read=165675047, gc_user=0x57e19ef8) at glxcurrent.c:228
        gc = 0x57e19ef8
        oldGC = 0x57e19ef8
#12 0xf754ce75 in glXMakeCurrent (dpy=0x57c16f30, draw=165675047, gc=0x57e19ef8) at glxcurrent.c:274
No locals.
#13 0xeb48addb in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
No symbol table info available.
#14 0xeb48dd0e in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
No symbol table info available.
#15 0xeb49d81d in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/vgui2_s.so
No symbol table info available.
#16 0xefd74193 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
No symbol table info available.
#17 0xefd76946 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
No symbol table info available.
#18 0xefd77f16 in ?? () from /home/chris/.local/share/Steam/ubuntu12_32/steamui.so
No symbol table info available.
#19 0x5663d660 in RunSteam(int, char**, bool) ()
No symbol table info available.
#20 0x5663e5f3 in ?? ()
No symbol table info available.
#21 0x56629aac in ?? ()
No symbol table info available.
#22 0xf799a1d3 in __libc_start_main () from /usr/lib32/libc.so.6
No symbol table info available.
#23 0x5662d159 in _start ()
No symbol table info available.


On 20.07.2017 20:26, Charmaine Lee wrote:
> With this patch, the st manager will maintain a hash table for
> the active framebuffer interface objects. A destroy_drawable interface
> is added to allow the state tracker to notify the st manager to remove
> the associated framebuffer interface object from the hash table,
> so the associated framebuffer and its resources can be deleted
> at framebuffers purge time.
>
> Fixes bug 101829 "read-after-free in st_framebuffer_validate"
>
> Tested-by: Brad King <brad.king at kitware.com>
> Tested-by: Gert Wollny <gw.fossdev at gmail.com>
> ---
>  src/gallium/include/state_tracker/st_api.h    |  7 ++
>  src/gallium/state_trackers/dri/dri_drawable.c |  6 +-
>  src/gallium/state_trackers/glx/xlib/xm_api.c  |  5 ++
>  src/gallium/state_trackers/glx/xlib/xm_st.c   |  2 +
>  src/gallium/state_trackers/wgl/stw_st.c       |  6 +-
>  src/mesa/state_tracker/st_manager.c           | 95 ++++++++++++++++++++++++++-
>  src/mesa/state_tracker/st_manager.h           |  5 ++
>  7 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/src/gallium/include/state_tracker/st_api.h b/src/gallium/include/state_tracker/st_api.h
> index 30a4866..9b660f7 100644
> --- a/src/gallium/include/state_tracker/st_api.h
> +++ b/src/gallium/include/state_tracker/st_api.h
> @@ -552,6 +552,13 @@ struct st_api
>      * Get the currently bound context in the calling thread.
>      */
>     struct st_context_iface *(*get_current)(struct st_api *stapi);
> +
> +   /**
> +    * Notify the st manager the framebuffer interface object
> +    * is no longer valid.
> +    */
> +   void (*destroy_drawable)(struct st_api *stapi,
> +                            struct st_framebuffer_iface *stfbi);
>  };
>
>  /**
> diff --git a/src/gallium/state_trackers/dri/dri_drawable.c b/src/gallium/state_trackers/dri/dri_drawable.c
> index 0cfdc30..c7df0f6 100644
> --- a/src/gallium/state_trackers/dri/dri_drawable.c
> +++ b/src/gallium/state_trackers/dri/dri_drawable.c
> @@ -169,6 +169,8 @@ void
>  dri_destroy_buffer(__DRIdrawable * dPriv)
>  {
>     struct dri_drawable *drawable = dri_drawable(dPriv);
> +   struct dri_screen *screen = drawable->screen;
> +   struct st_api *stapi = screen->st_api;
>     int i;
>
>     pipe_surface_reference(&drawable->drisw_surface, NULL);
> @@ -180,7 +182,9 @@ dri_destroy_buffer(__DRIdrawable * dPriv)
>
>     swap_fences_unref(drawable);
>
> -   drawable->base.ID = 0;
> +   /* Notify the st manager that this drawable is no longer valid */
> +   stapi->destroy_drawable(stapi, &drawable->base);
> +
>     FREE(drawable);
>  }
>
> diff --git a/src/gallium/state_trackers/glx/xlib/xm_api.c b/src/gallium/state_trackers/glx/xlib/xm_api.c
> index 881dd44..e4b1e9d 100644
> --- a/src/gallium/state_trackers/glx/xlib/xm_api.c
> +++ b/src/gallium/state_trackers/glx/xlib/xm_api.c
> @@ -595,6 +595,11 @@ xmesa_free_buffer(XMesaBuffer buffer)
>            */
>           b->ws.drawable = 0;
>
> +         /* Notify the st manager that the associated framebuffer interface
> +          * object is no longer valid.
> +          */
> +         stapi->destroy_drawable(stapi, buffer->stfb);
> +
>           /* XXX we should move the buffer to a delete-pending list and destroy
>            * the buffer until it is no longer current.
>            */
> diff --git a/src/gallium/state_trackers/glx/xlib/xm_st.c b/src/gallium/state_trackers/glx/xlib/xm_st.c
> index 9e30efa..6a0f4aa 100644
> --- a/src/gallium/state_trackers/glx/xlib/xm_st.c
> +++ b/src/gallium/state_trackers/glx/xlib/xm_st.c
> @@ -273,6 +273,7 @@ xmesa_st_framebuffer_flush_front(struct st_context_iface *stctx,
>     return ret;
>  }
>
> +static uint32_t xmesa_stfbi_ID = 0;
>
>  struct st_framebuffer_iface *
>  xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer b)
> @@ -302,6 +303,7 @@ xmesa_create_st_framebuffer(XMesaDisplay xmdpy, XMesaBuffer b)
>     stfbi->visual = &xstfb->stvis;
>     stfbi->flush_front = xmesa_st_framebuffer_flush_front;
>     stfbi->validate = xmesa_st_framebuffer_validate;
> +   stfbi->ID = p_atomic_inc_return(&xmesa_stfbi_ID);
>     p_atomic_set(&stfbi->stamp, 1);
>     stfbi->st_manager_private = (void *) xstfb;
>
> diff --git a/src/gallium/state_trackers/wgl/stw_st.c b/src/gallium/state_trackers/wgl/stw_st.c
> index c2844b0..85a8b17 100644
> --- a/src/gallium/state_trackers/wgl/stw_st.c
> +++ b/src/gallium/state_trackers/wgl/stw_st.c
> @@ -256,7 +256,11 @@ stw_st_destroy_framebuffer_locked(struct st_framebuffer_iface *stfb)
>     for (i = 0; i < ST_ATTACHMENT_COUNT; i++)
>        pipe_resource_reference(&stwfb->textures[i], NULL);
>
> -   stwfb->base.ID = 0;
> +   /* Notify the st manager that the framebuffer interface is no
> +    * longer valid.
> +    */
> +   stw_dev->stapi->destroy_drawable(stw_dev->stapi, &stwfb->base);
> +
>     FREE(stwfb);
>  }
>
> diff --git a/src/mesa/state_tracker/st_manager.c b/src/mesa/state_tracker/st_manager.c
> index cb816de..ebc7ca8 100644
> --- a/src/mesa/state_tracker/st_manager.c
> +++ b/src/mesa/state_tracker/st_manager.c
> @@ -38,6 +38,7 @@
>  #include "main/fbobject.h"
>  #include "main/renderbuffer.h"
>  #include "main/version.h"
> +#include "util/hash_table.h"
>  #include "st_texture.h"
>
>  #include "st_context.h"
> @@ -59,6 +60,10 @@
>  #include "util/u_surface.h"
>  #include "util/list.h"
>
> +struct hash_table;
> +static struct hash_table *st_fbi_ht; /* framebuffer iface objects hash table */
> +
> +static mtx_t st_mutex = _MTX_INITIALIZER_NP;
>
>  /**
>   * Map an attachment to a buffer index.
> @@ -490,6 +495,76 @@ st_framebuffer_reference(struct st_framebuffer **ptr,
>     _mesa_reference_framebuffer((struct gl_framebuffer **) ptr, fb);
>  }
>
> +
> +static uint32_t
> +st_framebuffer_iface_hash(const void *key)
> +{
> +   return (uintptr_t)key;
> +}
> +
> +
> +static bool
> +st_framebuffer_iface_equal(const void *a, const void *b)
> +{
> +   return (struct st_framebuffer_iface *)a == (struct st_framebuffer_iface *)b;
> +}
> +
> +
> +static boolean
> +st_framebuffer_iface_lookup(const struct st_framebuffer_iface *stfbi)
> +{
> +   struct hash_entry *entry;
> +
> +   mtx_lock(&st_mutex);
> +   entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
> +   mtx_unlock(&st_mutex);
> +
> +   return entry != NULL;
> +}
> +
> +
> +static boolean
> +st_framebuffer_iface_insert(struct st_framebuffer_iface *stfbi)
> +{
> +   struct hash_entry *entry;
> +
> +   mtx_lock(&st_mutex);
> +   entry = _mesa_hash_table_insert(st_fbi_ht, stfbi, stfbi);
> +   mtx_unlock(&st_mutex);
> +
> +   return entry != NULL;
> +}
> +
> +
> +static void
> +st_framebuffer_iface_remove(struct st_framebuffer_iface *stfbi)
> +{
> +   struct hash_entry *entry;
> +
> +   mtx_lock(&st_mutex);
> +   entry = _mesa_hash_table_search(st_fbi_ht, stfbi);
> +   if (!entry)
> +      goto unlock;
> +
> +   _mesa_hash_table_remove(st_fbi_ht, entry);
> +
> +unlock:
> +   mtx_unlock(&st_mutex);
> +}
> +
> +
> +/**
> + * The framebuffer interface object is no longer valid.
> + * Remove the object from the framebuffer interface hash table.
> + */
> +static void
> +st_api_destroy_drawable(struct st_api *stapi,
> +                        struct st_framebuffer_iface *stfbi)
> +{
> +   st_framebuffer_iface_remove(stfbi);
> +}
> +
> +
>  /**
>   * Purge the winsys buffers list to remove any references to
>   * non-existing framebuffer interface objects.
> @@ -506,7 +581,7 @@ st_framebuffers_purge(struct st_context *st)
>         * and unreference the framebuffer object, so its resources can be
>         * deleted.
>         */
> -      if (stfb->iface->ID != stfb->iface_ID) {
> +      if (!st_framebuffer_iface_lookup(stfb->iface)) {
>           LIST_DEL(&stfb->head);
>           st_framebuffer_reference(&stfb, NULL);
>        }
> @@ -810,6 +885,14 @@ st_framebuffer_reuse_or_create(struct st_context *st,
>        cur = st_framebuffer_create(st, stfbi);
>
>        if (cur) {
> +         /* add the referenced framebuffer interface object to
> +          * the framebuffer interface object hash table.
> +          */
> +         if (!st_framebuffer_iface_insert(stfbi)) {
> +            st_framebuffer_reference(&cur, NULL);
> +            return NULL;
> +         }
> +
>           /* add to the context's winsys buffers list */
>           LIST_ADD(&cur->head, &st->winsys_buffers);
>
> @@ -881,6 +964,8 @@ st_api_make_current(struct st_api *stapi, struct st_context_iface *stctxi,
>  static void
>  st_api_destroy(struct st_api *stapi)
>  {
> +   _mesa_hash_table_destroy(st_fbi_ht, NULL);
> +   mtx_destroy(&st_mutex);
>  }
>
>  /**
> @@ -1015,10 +1100,18 @@ static const struct st_api st_gl_api = {
>     .create_context = st_api_create_context,
>     .make_current = st_api_make_current,
>     .get_current = st_api_get_current,
> +   .destroy_drawable = st_api_destroy_drawable,
>  };
>
>  struct st_api *
>  st_gl_api_create(void)
>  {
> +   /* Create a hash table for all the framebuffer interface objects */
> +
> +   mtx_init(&st_mutex, mtx_plain);
> +   st_fbi_ht = _mesa_hash_table_create(NULL,
> +                                       st_framebuffer_iface_hash,
> +                                       st_framebuffer_iface_equal);
> +
>     return (struct st_api *) &st_gl_api;
>  }
> diff --git a/src/mesa/state_tracker/st_manager.h b/src/mesa/state_tracker/st_manager.h
> index 68adf2f..c54f29e 100644
> --- a/src/mesa/state_tracker/st_manager.h
> +++ b/src/mesa/state_tracker/st_manager.h
> @@ -34,6 +34,7 @@
>
>  struct st_context;
>  struct st_framebuffer;
> +struct st_framebuffer_interface;
>
>  void
>  st_manager_flush_frontbuffer(struct st_context *st);
> @@ -48,4 +49,8 @@ st_manager_add_color_renderbuffer(struct st_context *st, struct gl_framebuffer *
>  void
>  st_framebuffer_reference(struct st_framebuffer **ptr,
>                           struct st_framebuffer *stfb);
> +
> +void
> +st_framebuffer_interface_destroy(struct st_framebuffer_interface *stfbi);
> +
>  #endif /* ST_MANAGER_H */
>

_______________________________________________
mesa-dev mailing list
mesa-dev at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list