[vlc-devel] [PATCH 09/12] libvlc: media: fix leak if it has subitems

[Rémi Denis-Courmont]

Le lundi 19 janvier 2015, 11:22:51 Thomas Guillem a écrit :
> Don't call libvlc_media_list_set_media from media since it causes
> p_md->p_subitems to retain p_md while p_md is already retaining
> p_md->p_subitems, therefore these 2 objects won't be releasable.
> 
> Add a new internal function: _libvlc_media_list_set_media that set a media
> without retaining it.
> ---
>  lib/media.c               |  2 +-
>  lib/media_list.c          | 18 +++++++++++++++++-
>  lib/media_list_internal.h |  5 +++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/media.c b/lib/media.c
> index c73c5b1..cee1190 100644
> --- a/lib/media.c
> +++ b/lib/media.c
> @@ -115,7 +115,7 @@ static libvlc_media_list_t *media_get_subitems(
> libvlc_media_t * p_md ) if( p_md->p_subitems != NULL )
>          {
>              p_md->p_subitems->b_read_only = true;
> -            libvlc_media_list_set_media( p_md->p_subitems, p_md );
> +            libvlc_media_list_internal_set_media( p_md->p_subitems, p_md );
> }
>      }
>      p_subitems = p_md->p_subitems;
> diff --git a/lib/media_list.c b/lib/media_list.c
> index dff4edb..fa07d8d 100644
> --- a/lib/media_list.c
> +++ b/lib/media_list.c
> @@ -184,6 +184,7 @@ libvlc_media_list_new( libvlc_instance_t * p_inst )
>      assert( p_mlist->items.i_count == 0 );
>      p_mlist->i_refcount = 1;
>      p_mlist->p_md = NULL;
> +    p_mlist->p_internal_md = NULL;
> 
>      return p_mlist;
>  }
> @@ -285,12 +286,27 @@ void libvlc_media_list_set_media( libvlc_media_list_t
> * p_mlist,
> 
>  {
>      vlc_mutex_lock( &p_mlist->object_lock );
> +    if( p_mlist->p_internal_md )
> +    {
> +        vlc_mutex_unlock( &p_mlist->object_lock );
> +        return;
> +    }
>      libvlc_media_release( p_mlist->p_md );
>      libvlc_media_retain( p_md );
>      p_mlist->p_md = p_md;
>      vlc_mutex_unlock( &p_mlist->object_lock );
>  }
> 
> +/* LibVLC internal version: used only by media */
> +void libvlc_media_list_internal_set_media( libvlc_media_list_t * p_mlist,
> +                                           libvlc_media_t * p_md )
> +
> +{
> +    vlc_mutex_lock( &p_mlist->object_lock );
> +    p_mlist->p_internal_md = p_md;
> +    vlc_mutex_unlock( &p_mlist->object_lock );

This looks very fishy, taking a lock just to inconditionally set a variable.

> +}
> +
>  /**************************************************************************
> *       media (Public)
>   *
> @@ -306,7 +322,7 @@ libvlc_media_list_media( libvlc_media_list_t * p_mlist )
> libvlc_media_t *p_md;
> 
>      vlc_mutex_lock( &p_mlist->object_lock );
> -    p_md = p_mlist->p_md;
> +    p_md = p_mlist->p_internal_md ? p_mlist->p_internal_md : p_mlist->p_md;
> if( p_md )
>          libvlc_media_retain( p_md );
>      vlc_mutex_unlock( &p_mlist->object_lock );
> diff --git a/lib/media_list_internal.h b/lib/media_list_internal.h
> index 9c2412b..832ae13 100644
> --- a/lib/media_list_internal.h
> +++ b/lib/media_list_internal.h
> @@ -44,6 +44,7 @@ struct libvlc_media_list_t
>      vlc_mutex_t                 refcount_lock;
>      libvlc_media_t * p_md; /* The media from which the
>                                         * mlist comes, if any. */
> +    libvlc_media_t * p_internal_md; /* media set from media.c */
>      vlc_array_t                items;
> 
>      /* This indicates if this media list is read-only
> @@ -52,6 +53,10 @@ struct libvlc_media_list_t
>  };
> 
>  /* Media List */
> +void libvlc_media_list_internal_set_media(
> +        libvlc_media_list_t * p_mlist,
> +        libvlc_media_t * p_md );
> +
>  void libvlc_media_list_internal_add_media(
>          libvlc_media_list_t * p_mlist,
>          libvlc_media_t * p_md );