[vlc-devel] [PATCH] picture_pool: do not unconditionally free the pictures

[Rémi Denis-Courmont]

On Thu, 11 Jul 2013 08:20:29 +0200, Luca Barbato <lu_zero at gentoo.org>
wrote:
> If a picture is still referenced would be double-freed.

I guess Laurent's rationale was that a picture cannot be referenced by the
time the pool is deleted.

> ---
> 
> If there are places that assume that picture_pool_Delete must wipe all
> the references doesn't matter what we can add a second function.
> 
>  src/misc/picture_pool.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/src/misc/picture_pool.c b/src/misc/picture_pool.c
> index 14db3b6..356aff3 100644
> --- a/src/misc/picture_pool.c
> +++ b/src/misc/picture_pool.c
> @@ -195,11 +195,6 @@ void picture_pool_Delete(picture_pool_t *pool)
>              assert(vlc_atomic_get(&picture->gc.refcount) == 0);
>              assert(!pool->picture_reserved[i]);
> 
> -            /* Restore old release callback */
> -            vlc_atomic_set(&picture->gc.refcount, 1);
> -            picture->gc.pf_destroy = gc_sys->destroy;
> -            picture->gc.p_sys      = gc_sys->destroy_sys;
> -

You can't do that. The original saved garbage collection context will be
lost when picture_pool_Delete() returns. So instead of actually freeing the
picture, the destroy callback will try to access the already freed pool
internals, and (if it does not crash) leak the picture forever.

And last but not least, the video output plugins assume that pictures are
destroyed when they destroy their pool. Video output pictures must
obviously be destroyed before the video output that knows how to allocate
and deallocate them. (This limitation could be avoided on X11: you would
need to add support for SysV SHM pictures to the VLC core. I on't know
about platforms though.)

>              picture_Release(picture);
> 
>              free(gc_sys);

-- 
Rémi Denis-Courmont
Sent from my collocated server