[MPlayer-dev-eng] [patch 5/5] Fix muxer memory leak

[Tobias Diedrich]

Reimar Döffinger wrote:
> On Tue, Jan 04, 2011 at 09:35:06PM +0100, Tobias Diedrich wrote:
> > Original patch by Sang-Uok Kum.
> > 
> > Signed-off-by: Tobias Diedrich <ranma at google.com>
> > 
> > Index: mplayer-patchset1/libmpdemux/muxer.c
> > ===================================================================
> > --- mplayer-patchset1.orig/libmpdemux/muxer.c	2010-12-21 21:03:16.199700000 +0100
> > +++ mplayer-patchset1/libmpdemux/muxer.c	2010-12-21 21:05:11.009615000 +0100
> > @@ -140,6 +140,8 @@
> >            s->timer = buf->dts;
> >            s->buffer = buf->buffer;
> >            s->muxer->cont_write_chunk(s, buf->len, buf->flags, buf->dts, buf->pts);
> > +          free(buf->buffer);
> > +          buf->buffer = NULL;
> 
> I suspect the original idea was to reuse the allocated memory if
> possible, but that is sure better than a memleak.

> > @@ -148,6 +150,7 @@
> >  
> >          free(s->muxer->muxbuf);
> >          s->muxer->muxbuf_num = 0;
> > +        s->muxer->muxbuf = NULL;
> 
> That is not a memleak fix, though it is a fix.
> However I see several more issues, for example I think the code still
> can end up with a memleak and some unwritten final packets, there needs
> to be some function that flushes all remaining buffered frames at the
> end.
> And after that it would be better to _not_ free muxbuf here but instead
> give realloc a chance to reuse the memory area.

Actually the realloc is for s->muxer->muxbuf, which already had the
free.  The missing free is for the buf->buffer chunk buffers.
The way I see it the muxer works like this:

1)
When the muxer is first allocated in muxer_new_muxer(),
muxbuf_skip_buffer is initialized to 0 by calloc().
2)
muxer_write_chunk() then allocates memory buffers for all writes
until and does not actually call cont_write_chunk() until all
streams have at least one chunk buffered.
3)
On the next muxer_write_chunk() muxbuf_skip_buffer is then set to 1
and it calls muxer_stream_fix_parameters(), muxer_write_header() and
proceeds to flush those buffers, but fails to frees the buffers.
4)
Since muxbuf_skip_buffer is now 1, the buffering logic is bypassed.

So to me it seems like the
> > +          free(buf->buffer);
> > +          buf->buffer = NULL;
is a valid fix unless I'm missing something.

Cheers,

-- 
Tobias						PGP: http://8ef7ddba.uguu.de