[vlc-devel] [PATCH] Add addons management API

[Rémi Denis-Courmont]

Le vendredi 7 février 2014, 23:41:19 Francois Cartegnie a écrit :
(...)
> +#define INIT_QUEUE( name ) \
> +    p_manager->p_priv->name.b_live = false;\
> +    p_manager->p_priv->name.p_thread = NULL;\
> +    vlc_mutex_init( &p_manager->p_priv->name.lock );\
> +    ARRAY_INIT( p_manager->p_priv->name.entries );

There is no warranty that vlc_thread_t is a pointer type, and in particular 
there is no warranty that NULL can be used as an error value.

In particular, on POSIX platforms, vlc_thread_t is pthread_t. There is no 
warranty that pthread_t is a pointer type, or even any arithmetic type at all.

(...)
> +void addons_manager_Delete( addons_manager_t *p_manager )
> +{
> +    if ( p_manager->p_priv->finder.b_live )
> +        vlc_cancel( *p_manager->p_priv->finder.p_thread );
> +    if ( p_manager->p_priv->installer.b_live )
> +        vlc_cancel( *p_manager->p_priv->installer.p_thread );

To call vlc_cancel() on a detached thread, the thread must still be running. 
Otherwise, the thread handle is no longer valid. It seems to me that there is 
no such warranty and thus this is potentially a use-after-free bug.

> +
> +    vlc_event_manager_fini( p_manager->p_event_manager );
> +
> +#define FREE_QUEUE( name ) \
> +    vlc_mutex_lock( &p_manager->p_priv->name.lock );\
> +    FOREACH_ARRAY( addon_entry_t *p_entry, p_manager->p_priv->name.entries
> )\ +        addon_entry_Release( p_entry );\
> +    FOREACH_END();\
> +    ARRAY_RESET( p_manager->p_priv->name.entries );\
> +    vlc_mutex_unlock( &p_manager->p_priv->name.lock );\
> +    vlc_mutex_destroy( &p_manager->p_priv->name.lock );
> +
> +    FREE_QUEUE( finder )
> +    FREE_QUEUE( installer )
> +    free( p_manager->p_priv->finder.psz_uri_hint );
> +
> +    free( p_manager->p_priv );
> +    free( p_manager->p_event_manager );
> +    free( p_manager );
> +}

So nack for me.

-- 
Реми Денис-Курмон
http://www.remlab.net/