vlc | branch: master | Rémi Denis-Courmont <[email protected]> | 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() _______________________________________________ vlc-commits mailing list [email protected] https://mailman.videolan.org/listinfo/vlc-commits
