[vlc-commits] event: remove recursive deletion
Rémi Denis-Courmont
git at videolan.org
Tue May 16 22:05:54 CEST 2017
vlc | branch: master | Rémi Denis-Courmont <remi at remlab.net> | Tue May 16 23:03:03 2017 +0300| [5c2c82edb99c51ea8d882c289007cc4bef874df7] | committer: Rémi Denis-Courmont
event: remove recursive deletion
In theory, vlc_event_detach() can be called from within the event
handler. In practice, callers of vlc_event_detach() expect that the
event handler is not pending after the function returns. This would not
work if recursion actually occurred, it would lead to use-after-free.
This removes recursion, including memory allocation, copying and missing
error handling in event sending.
> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=5c2c82edb99c51ea8d882c289007cc4bef874df7
---
include/vlc_events.h | 9 +----
src/misc/events.c | 105 +++++----------------------------------------------
2 files changed, 10 insertions(+), 104 deletions(-)
diff --git a/include/vlc_events.h b/include/vlc_events.h
index 6a508289b5..667d32bf9c 100644
--- a/include/vlc_events.h
+++ b/include/vlc_events.h
@@ -111,20 +111,13 @@ typedef enum vlc_event_type_t {
typedef struct vlc_event_listeners_group_t
{
DECL_ARRAY(struct vlc_event_listener_t *) listeners;
-
- /* Used in vlc_event_send() to make sure to behave
- Correctly when vlc_event_detach was called during
- a callback */
- bool b_sublistener_removed;
-
} vlc_event_listeners_group_t;
/* Event manager type */
typedef struct vlc_event_manager_t
{
void * p_obj;
- vlc_mutex_t object_lock;
- vlc_mutex_t event_sending_lock;
+ vlc_mutex_t lock;
vlc_event_listeners_group_t events[vlc_InputItemPreparseEnded + 1];
} vlc_event_manager_t;
diff --git a/src/misc/events.c b/src/misc/events.c
index 508b4c5983..a0efbda6ad 100644
--- a/src/misc/events.c
+++ b/src/misc/events.c
@@ -52,26 +52,6 @@ typedef struct vlc_event_listener_t
vlc_event_callback_t pf_callback;
} vlc_event_listener_t;
-static bool
-listeners_are_equal( vlc_event_listener_t * listener1,
- vlc_event_listener_t * listener2 )
-{
- return listener1->pf_callback == listener2->pf_callback &&
- listener1->p_user_data == listener2->p_user_data;
-}
-
-static bool
-group_contains_listener( vlc_event_listeners_group_t * group,
- vlc_event_listener_t * searched_listener )
-{
- vlc_event_listener_t * listener;
- FOREACH_ARRAY( listener, group->listeners )
- if( listeners_are_equal(searched_listener, listener) )
- return true;
- FOREACH_END()
- return false;
-}
-
/*****************************************************************************
*
*****************************************************************************/
@@ -86,15 +66,7 @@ group_contains_listener( vlc_event_listeners_group_t * group,
void vlc_event_manager_init( vlc_event_manager_t * p_em, void * p_obj )
{
p_em->p_obj = p_obj;
- vlc_mutex_init( &p_em->object_lock );
-
- /* We need a recursive lock here, because we need to be able
- * to call libvlc_event_detach even if vlc_event_send is in
- * the call frame.
- * This ensures that after libvlc_event_detach, the callback
- * will never gets triggered.
- * */
- vlc_mutex_init_recursive( &p_em->event_sending_lock );
+ vlc_mutex_init( &p_em->lock );
for( size_t i = 0; i < ARRAY_SIZE(p_em->events); i++ )
ARRAY_INIT( p_em->events[i].listeners );
@@ -107,8 +79,7 @@ void vlc_event_manager_fini( vlc_event_manager_t * p_em )
{
struct vlc_event_listener_t * listener;
- vlc_mutex_destroy( &p_em->object_lock );
- vlc_mutex_destroy( &p_em->event_sending_lock );
+ vlc_mutex_destroy( &p_em->lock );
for( size_t i = 0; i < ARRAY_SIZE(p_em->events); i++ )
{
@@ -129,69 +100,17 @@ void vlc_event_send( vlc_event_manager_t * p_em,
{
vlc_event_listeners_group_t *slot = &p_em->events[p_event->type];
vlc_event_listener_t * listener;
- vlc_event_listener_t * array_of_cached_listeners = NULL;
- vlc_event_listener_t * cached_listener;
-
- int i, i_cached_listeners = 0;
/* Fill event with the sending object now */
p_event->p_obj = p_em->p_obj;
- vlc_mutex_lock( &p_em->event_sending_lock ) ;
- vlc_mutex_lock( &p_em->object_lock );
+ vlc_mutex_lock( &p_em->lock ) ;
- if( slot->listeners.i_size <= 0 )
- {
- vlc_mutex_unlock( &p_em->object_lock );
- vlc_mutex_unlock( &p_em->event_sending_lock ) ;
- return;
- }
-
- /* Save the function to call */
- i_cached_listeners = slot->listeners.i_size;
- array_of_cached_listeners = malloc(
- sizeof(vlc_event_listener_t)*i_cached_listeners );
- if( unlikely(!array_of_cached_listeners) )
- abort();
-
- cached_listener = array_of_cached_listeners;
FOREACH_ARRAY( listener, slot->listeners )
- memcpy( cached_listener, listener, sizeof(vlc_event_listener_t) );
- cached_listener++;
+ listener->pf_callback( p_event, listener->p_user_data );
FOREACH_END()
- /* Track item removed from *this* thread, with a simple flag. Indeed
- * event_sending_lock is a recursive lock. This has the advantage of
- * allowing to remove an event listener from within a callback */
- slot->b_sublistener_removed = false;
-
- vlc_mutex_unlock( &p_em->object_lock );
-
- /* Call the function attached */
- cached_listener = array_of_cached_listeners;
-
- for( i = 0; i < i_cached_listeners; i++ )
- {
- if( slot->b_sublistener_removed )
- {
- /* If a callback was removed inside one of our callback, this gets
- * called */
- bool valid_listener;
- vlc_mutex_lock( &p_em->object_lock );
- valid_listener = group_contains_listener( slot, cached_listener );
- vlc_mutex_unlock( &p_em->object_lock );
- if( !valid_listener )
- {
- cached_listener++;
- continue;
- }
- }
- cached_listener->pf_callback( p_event, cached_listener->p_user_data );
- cached_listener++;
- }
- vlc_mutex_unlock( &p_em->event_sending_lock );
-
- free( array_of_cached_listeners );
+ vlc_mutex_unlock( &p_em->lock );
}
#undef vlc_event_attach
@@ -213,9 +132,9 @@ int vlc_event_attach( vlc_event_manager_t * p_em,
listener->p_user_data = p_user_data;
listener->pf_callback = pf_callback;
- vlc_mutex_lock( &p_em->object_lock );
+ vlc_mutex_lock( &p_em->lock );
ARRAY_APPEND( slot->listeners, listener );
- vlc_mutex_unlock( &p_em->object_lock );
+ vlc_mutex_unlock( &p_em->lock );
return VLC_SUCCESS;
}
@@ -231,24 +150,18 @@ void vlc_event_detach( vlc_event_manager_t *p_em,
vlc_event_listeners_group_t *slot = &p_em->events[event_type];
struct vlc_event_listener_t * listener;
- vlc_mutex_lock( &p_em->event_sending_lock );
- vlc_mutex_lock( &p_em->object_lock );
+ vlc_mutex_lock( &p_em->lock );
FOREACH_ARRAY( listener, slot->listeners )
if( listener->pf_callback == pf_callback &&
listener->p_user_data == p_user_data )
{
- /* Tell vlc_event_send, we did remove an item from that group,
- in case vlc_event_send is in our caller stack */
- slot->b_sublistener_removed = true;
-
/* that's our listener */
ARRAY_REMOVE( slot->listeners,
fe_idx /* This comes from the macro (and that's why
I hate macro) */ );
+ vlc_mutex_unlock( &p_em->lock );
free( listener );
- vlc_mutex_unlock( &p_em->event_sending_lock );
- vlc_mutex_unlock( &p_em->object_lock );
return;
}
FOREACH_END()
More information about the vlc-commits
mailing list