[Mesa-dev] [PATCH] st/mesa: add destroy_drawable interface
Brian Paul
brianp at vmware.com
Thu Jul 20 22:35:32 UTC 2017
Looks good to me, but you might want to wait a day to see if there's any
additional review feedback.
Reviewed-by: Brian Paul <brianp at vmware.com>
On 07/20/2017 12:26 PM, 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 */
>
More information about the mesa-dev
mailing list