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

[Stefano Sabatini]

On date Sunday 2011-12-18 01:22:21 +0100, Stefano Sabatini encoded:
> On date Sunday 2011-12-18 01:01:32 +0100, Stefano Sabatini encoded:
> > 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).
> 
> After more tinkering and supposing I'm not already sleeping: we should
> copy the pix_fmts list in the buffersink, free the original list in
> the lavfi device and free the copy in the buffersink uninit().
> 
> I'll try to write a patch for this tomorrow, but feel free to beat me
> at it.

Implemented this way, and applied this patch on top of it, thanks.
-- 
FFmpeg = Faithful and Fantastic Mean Problematic Everlasting Gnome