[FFmpeg-devel] [PATCH] lavd/lavfi: fix two memleaks.

[Stefano Sabatini]

On date Friday 2011-12-16 22:38:09 +0100, Michael Niedermayer encoded:
> On Fri, Dec 16, 2011 at 09:54:52PM +0100, Nicolas George wrote:
> > Le sextidi 26 frimaire, an CCXX, Michael Niedermayer a écrit :
> > > this might lead to use after free in sink_buffer.c if iam not misssing
> > > anything
> >
> > I looked into it before submitting, and I think that sink_buffer will not
> > use it after init(), which is called by avfilter_graph_config. And valgrind
> > did not show new errors after the new free.
> 
> My concern is that we pass a pointer into sink_buffer which keeps it
> and uses it in an exported function (its query_formats() that gets
> called by avfilter_graph_config())
> and we free it outside while sink_buffer retains this now invalid
> pointer.
> 

> AFAIK nothing calls query_formats() again and
> nothing uses that pointer except query_formats(). But i think it
> would be better if this pointer was set to NULL once it becomes
> unavailable or if it would be freed only once sink_buffer is freed

I agree with this.

In vsink_init() we have:

#if FF_API_OLD_VSINK_API
        buf->pixel_fmts = (const enum PixelFormat *)opaque;
#else
        params = (AVBufferSinkParams *)opaque;
        buf->pixel_fmts = params->pixel_fmts;
#endif

so the passed opaque data is referenced in the buffersink context (and
never freed). I suppose the safest approach is to require the caller
to allocate the opaque data (like it is right now), and ask buffersink
to free it (which is not doing, and leaking).
  
> >
> > Stefano may be more competent to check this is correct.
> 
> It would be great if stefano could comment too

Here I am!
-- 
FFmpeg = Forgiving Faithless MultiPurpose Educated Guru