From ee235a6b070bc386fa545f02f850624736e2b07e Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Mon, 25 Jul 2011 12:53:10 +0200 Subject: [PATCH] miniobject: avoid race in bufferpool release Avoid playing with the refcount to decide when a buffer has been recycled by the dispose function. The problem is that we then temporarily can have a buffer with a refcount > 1 being acquired from the pool, which is not writable. Instead use a simple boolean return value from the dispose function to inform the called that the object was recycled or not. --- gst/gstbuffer.c | 20 ++++++++++++-------- gst/gstminiobject.c | 13 ++++++------- gst/gstminiobject.h | 8 +++++--- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/gst/gstbuffer.c b/gst/gstbuffer.c index 91c72ad..ce1d1ee 100644 --- a/gst/gstbuffer.c +++ b/gst/gstbuffer.c @@ -363,18 +363,22 @@ _gst_buffer_copy (GstBuffer * buffer) /* the default dispose function revives the buffer and returns it to the * pool when there is a pool */ -static void +static gboolean _gst_buffer_dispose (GstBuffer * buffer) { GstBufferPool *pool; - if ((pool = buffer->pool) != NULL) { - /* keep the buffer alive */ - gst_buffer_ref (buffer); - /* return the buffer to the pool */ - GST_CAT_LOG (GST_CAT_BUFFER, "release %p to pool %p", buffer, pool); - gst_buffer_pool_release_buffer (pool, buffer); - } + /* no pool, do free */ + if ((pool = buffer->pool) == NULL) + return TRUE; + + /* keep the buffer alive */ + gst_buffer_ref (buffer); + /* return the buffer to the pool */ + GST_CAT_LOG (GST_CAT_BUFFER, "release %p to pool %p", buffer, pool); + gst_buffer_pool_release_buffer (pool, buffer); + + return FALSE; } static void diff --git a/gst/gstminiobject.c b/gst/gstminiobject.c index 9e6fd9b..9e83e8d 100644 --- a/gst/gstminiobject.c +++ b/gst/gstminiobject.c @@ -254,17 +254,16 @@ gst_mini_object_unref (GstMiniObject * mini_object) GST_MINI_OBJECT_REFCOUNT_VALUE (mini_object) - 1); if (G_UNLIKELY (g_atomic_int_dec_and_test (&mini_object->refcount))) { - /* At this point, the refcount of the object is 0. We increase the refcount - * here because if a subclass recycles the object and gives out a new - * reference we don't want to free the instance anymore. */ - gst_mini_object_ref (mini_object); + gboolean do_free; if (mini_object->dispose) - mini_object->dispose (mini_object); + do_free = mini_object->dispose (mini_object); + else + do_free = TRUE; - /* decrement the refcount again, if the subclass recycled the object we don't + /* if the subclass recycled the object (and returned FALSE) we don't * want to free the instance anymore */ - if (G_LIKELY (g_atomic_int_dec_and_test (&mini_object->refcount))) { + if (G_LIKELY (do_free)) { /* The weak reference stack is freed in the notification function */ if (mini_object->n_weak_refs) weak_refs_notify (mini_object); diff --git a/gst/gstminiobject.h b/gst/gstminiobject.h index 74d959f..630eb44 100644 --- a/gst/gstminiobject.h +++ b/gst/gstminiobject.h @@ -52,10 +52,12 @@ typedef GstMiniObject * (*GstMiniObjectCopyFunction) (const GstMiniObject *obj); * Function prototype for when a miniobject has lost its last refcount. * Implementation of the mini object are allowed to revive the * passed object by doing a gst_mini_object_ref(). If the object is not - * revived after the dispose function, the memory associated with the - * object is freed. + * revived after the dispose function, the function should return %TRUE + * and the memory associated with the object is freed. + * + * Returns: %TRUE if the object should be cleaned up. */ -typedef void (*GstMiniObjectDisposeFunction) (GstMiniObject *obj); +typedef gboolean (*GstMiniObjectDisposeFunction) (GstMiniObject *obj); /** * GstMiniObjectFreeFunction: * @obj: MiniObject to free -- 2.7.4