[vlc-devel] [vlc-commits] playlist tree: fix potential memleak

[Rémi Denis-Courmont]

I'm getting memory corruption, especially when exiting VLC, for instance:

==5072== Invalid read of size 4
==5072==    at 0x40F2B1A: vlc_atomic_add (atomic.c:46)
==5072==    by 0x405D40E: vlc_release (vlc_atomic.h:40)
==5072==    by 0x4078BF6: playlist_Destroy (engine.c:298)
==5072==    by 0x405FD56: libvlc_InternalCleanup (libvlc.c:1014)
==5072==    by 0x4031108: libvlc_release (core.c:108)
==5072==    by 0x8049209: main (vlc.c:249)
==5072==  Address 0x4ca4cd8 is 528 bytes inside a block of size 652 free'd
==5072==    at 0x4025102: realloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==5072==    by 0x40D969F: module_list_get (modules.c:360)
==5072==    by 0x40D97E9: vlc_module_load (modules.c:475)
==5072==    by 0x442464F: ???
==5072== 
==5072== 

7d84269bff28a7e173ef521a56fff5acca2590cf is the first bad commit
commit 7d84269bff28a7e173ef521a56fff5acca2590cf
Author: Rafaël Carré <rafael.carre at gmail.com>
Date:   Sun Aug 7 19:23:40 2011 -0400

    playlist tree: fix potential memleak
    
    also remove one level of indentation

:040000 040000 47f1fb139c9a5cc3b8424a8c25de8305cfb6c163 
840629d4f82cdabebfde778ba1b9ea604cfbd964 M      src

Please fix. This blocks any meaningful smoke testing.

Le lundi 8 août 2011 02:58:29 Rafaël Carré, vous avez écrit :
> vlc | branch: master | Rafaël Carré <rafael.carre at gmail.com> | Sun Aug  7
> 19:23:40 2011 -0400| [7d84269bff28a7e173ef521a56fff5acca2590cf] |
> committer: Rafaël Carré
> 
> playlist tree: fix potential memleak
> 
> also remove one level of indentation
> 
> > http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=7d84269bff28a7e173
> > ef521a56fff5acca2590cf
> 
> ---
> 
>  src/playlist/tree.c |   75
> +++++++++++++++++++++++--------------------------- 1 files changed, 35
> insertions(+), 40 deletions(-)
> 
> diff --git a/src/playlist/tree.c b/src/playlist/tree.c
> index fd7233a..e306572 100644
> --- a/src/playlist/tree.c
> +++ b/src/playlist/tree.c
> @@ -141,60 +141,55 @@ int playlist_NodeDelete( playlist_t *p_playlist,
> playlist_item_t *p_root, bool b_delete_items, bool b_force )
>  {
>      PL_ASSERT_LOCKED;
> -    int i;
> 
>      /* Delete the children */
> -    for( i = p_root->i_children - 1 ; i >= 0; i-- )
> -    {
> -        if( b_delete_items || p_root->pp_children[i]->i_children > -1 )
> -        {
> +    for( int i = p_root->i_children - 1 ; i >= 0; i-- )
> +        if( b_delete_items || p_root->pp_children[i]->i_children >= 0 )
>              playlist_NodeDelete( p_playlist, p_root->pp_children[i],
>                                   b_delete_items, b_force );
> -        }
> -    }
> +
>      /* Delete the node */
>      if( p_root->i_flags & PLAYLIST_RO_FLAG && !b_force )
> -    {
> +        return VLC_SUCCESS;
> +
> +    pl_priv(p_playlist)->b_reset_currently_playing = true;
> +
> +    int i;
> +    var_SetInteger( p_playlist, "playlist-item-deleted", p_root->i_id );
> +    ARRAY_BSEARCH( p_playlist->all_items, ->i_id, int, p_root->i_id, i );
> +    if( i != -1 ) {
> +        vlc_gc_decref(p_playlist->all_items.p_elems[i]->p_input);
> +        printf("deleting %d %p\n", i,
> p_playlist->all_items.p_elems[i]->p_input); +        ARRAY_REMOVE(
> p_playlist->all_items, i );
>      }
> -    else
> -    {
> -        pl_priv(p_playlist)->b_reset_currently_playing = true;
> 
> -        int i;
> -        var_SetInteger( p_playlist, "playlist-item-deleted", p_root->i_id
> ); -        ARRAY_BSEARCH( p_playlist->all_items, ->i_id, int,
> -                       p_root->i_id, i );
> +    if( p_root->i_children == -1 ) {
> +        ARRAY_BSEARCH( p_playlist->items,->i_id, int, p_root->i_id, i );
>          if( i != -1 )
> -            ARRAY_REMOVE( p_playlist->all_items, i );
> -
> -        if( p_root->i_children == -1 ) {
> -            ARRAY_BSEARCH( p_playlist->items,->i_id, int, p_root->i_id, i
> ); -            if( i != -1 )
> -                ARRAY_REMOVE( p_playlist->items, i );
> -        }
> +            ARRAY_REMOVE( p_playlist->items, i );
> +    }
> 
> -        /* Check if it is the current item */
> -        if( get_current_status_item( p_playlist ) == p_root )
> -        {
> -            /* Stop */
> -            playlist_Control( p_playlist, PLAYLIST_STOP, pl_Locked );
> -            msg_Info( p_playlist, "stopping playback" );
> -            /* This item can't be the next one to be played ! */
> -            set_current_status_item( p_playlist, NULL );
> -        }
> +    /* Check if it is the current item */
> +    if( get_current_status_item( p_playlist ) == p_root )
> +    {
> +        /* Stop */
> +        playlist_Control( p_playlist, PLAYLIST_STOP, pl_Locked );
> +        msg_Info( p_playlist, "stopping playback" );
> +        /* This item can't be the next one to be played ! */
> +        set_current_status_item( p_playlist, NULL );
> +    }
> 
> -        ARRAY_BSEARCH( p_playlist->current,->i_id, int, p_root->i_id, i );
> -        if( i != -1 )
> -            ARRAY_REMOVE( p_playlist->current, i );
> +    ARRAY_BSEARCH( p_playlist->current,->i_id, int, p_root->i_id, i );
> +    if( i != -1 )
> +        ARRAY_REMOVE( p_playlist->current, i );
> 
> -        PL_DEBUG( "deleting item `%s'", p_root->p_input->psz_name );
> +    PL_DEBUG( "deleting item `%s'", p_root->p_input->psz_name );
> 
> -        /* Remove the item from its parent */
> -        if( p_root->p_parent )
> -            playlist_NodeRemoveItem( p_playlist, p_root, p_root->p_parent
> ); +    /* Remove the item from its parent */
> +    if( p_root->p_parent )
> +        playlist_NodeRemoveItem( p_playlist, p_root, p_root->p_parent );
> 
> -        playlist_ItemRelease( p_root );
> -    }
> +    playlist_ItemRelease( p_root );
>      return VLC_SUCCESS;
>  }
> 
> 
> _______________________________________________
> vlc-commits mailing list
> vlc-commits at videolan.org
> http://mailman.videolan.org/listinfo/vlc-commits


-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis