[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