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

Marek Olšák maraeo at gmail.com
Mon Jul 24 22:16:07 UTC 2017


Hi Charmaine,

Which commits do I need to revert in 17.2 to unbreak Steam on Mesa? I
guess it's more than one.

Thanks,
Marek

On Sat, Jul 22, 2017 at 3:05 AM, Charmaine Lee <charmainel at vmware.com> wrote:
>
> 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
> _______________________________________________
> 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