[FFmpeg-devel] [PATCH] Fix for memory leak in mov format

[Art Clarke]

On Fri, May 16, 2008 at 9:37 AM, Art Clarke <aclarke at vlideshow.com> wrote:

> On Thu, May 15, 2008 at 9:27 AM, Art Clarke <aclarke at vlideshow.com> wrote:
>
>> Hi folks,
>>
>> This is my first patch submitted, so please let me know if you need
>> additional information or if I screwed up the format in some way.
>>
>> Description of problem:
>> The mov demuxer allocates some private data on each AVStream it adds, but
>> never frees the data.
>>
>> How to reproduce:
>> Use av_open_input_file() to open a .mov file, then call
>> av_close_input_file().  Run program through a memory checking tool (e.g.
>> valgrind).
>>
>> Description of fix:
>> When closing a .mov format file, free the allocated MOVContext object and
>> null the AVFormatContext->priv_data value.
>>
>> Pre-Patch Results (SVN r13158)
>> "make test" tests pass (assuming "configure" has no options passed in).
>>
>> Valgrind results (test opens .mov file, decodes all audio and video,
>> remixes audio in special ways, and then encodes a new FLV file):
>> valgrind --leak-check=full ../aaffmpeg_test
>> com::vlideshow::aaffmpeg::AudioSamplesMixerTest 8
>> ==21759== Memcheck, a memory error detector.
>> ==21759== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
>> ==21759== Using LibVEX rev 1804, a library for dynamic binary translation.
>> ==21759== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
>> ==21759== Using valgrind-3.3.0, a dynamic binary instrumentation
>> framework.
>> ==21759== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
>> ==21759== For more details, rerun with: -v
>> ==21759==
>> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x411e540]negative ctts, ignoring
>>
>> com::vlideshow::aaffmpeg::AudioSamplesMixerTest: .
>>
>> tests summary: ok:1
>> ==21759==
>> ==21759== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 34 from 1)
>> ==21759== malloc/free: in use at exit: 360 bytes in 3 blocks.
>> ==21759== malloc/free: 29,788 allocs, 29,785 frees, 12,794,101 bytes
>> allocated.
>> ==21759== For counts of detected errors, rerun with: -v
>> ==21759== searching for pointers to 3 not-freed blocks.
>> ==21759== checked 1,441,900 bytes.
>> ==21759==
>> ==21759== 360 bytes in 3 blocks are definitely lost in loss record 1 of 1
>> ==21759==    at 0x4004802: memalign (vg_replace_malloc.c:460)
>> ==21759==    by 0x462FDCA: av_malloc (mem.c:61)
>> ==21759==    by 0x462FE59: av_mallocz (mem.c:134)
>> ==21759==    by 0x40E7E0F: mov_read_trak (mov.c:1253)
>> ==21759==    by 0x40E4FC5: mov_read_default (mov.c:215)
>> ==21759==    by 0x40E5A82: mov_read_moov (mov.c:455)
>> ==21759==    by 0x40E4FC5: mov_read_default (mov.c:215)
>> ==21759==    by 0x40E922C: mov_read_header (mov.c:1736)
>> ==21759==    by 0x40A4F0B: av_open_input_stream (utils.c:395)
>> ==21759==    by 0x40A51D7: av_open_input_file (utils.c:481)
>> ==21759==    by 0x4059B7B:
>> com::vlideshow::aaffmpeg::Container::openInputURL(char const*,
>> com::vlideshow::aaffmpeg::IContainerFormat*) (Container.cpp:91)
>> ==21759==    by 0x4059F4D: com::vlideshow::aaffmpeg::Container::open(char
>> const*, com::vlideshow::aaffmpeg::IContainer::Type,
>> com::vlideshow::aaffmpeg::IContainerFormat*) (Container.cpp:66)
>> ==21759==
>> ==21759== LEAK SUMMARY:
>> ==21759==    definitely lost: 360 bytes in 3 blocks.
>> ==21759==      possibly lost: 0 bytes in 0 blocks.
>> ==21759==    still reachable: 0 bytes in 0 blocks.
>> ==21759==         suppressed: 0 bytes in 0 blocks.
>>
>> Post-Patch results:
>> "make test" tests pass (assuming "configure has no options passed in).
>>
>> Valgrind results (test opens .mov file, decodes all audio and video,
>> remixes audio in special ways, and then encodes a new FLV file)
>> valgrind --leak-check=full ../aaffmpeg_test
>> com::vlideshow::aaffmpeg::AudioSamplesMixerTest 8
>> ==21917== Memcheck, a memory error detector.
>> ==21917== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
>> ==21917== Using LibVEX rev 1804, a library for dynamic binary translation.
>> ==21917== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
>> ==21917== Using valgrind-3.3.0, a dynamic binary instrumentation
>> framework.
>> ==21917== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
>> ==21917== For more details, rerun with: -v
>> ==21917==
>> [mov,mp4,m4a,3gp,3g2,mj2 @ 0x411e540]negative ctts, ignoring
>>
>> com::vlideshow::aaffmpeg::AudioSamplesMixerTest: .
>>
>> tests summary: ok:1
>> ==21917==
>> ==21917== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 34 from 1)
>> ==21917== malloc/free: in use at exit: 0 bytes in 0 blocks.
>> ==21917== malloc/free: 29,788 allocs, 29,788 frees, 12,794,101 bytes
>> allocated.
>> ==21917== For counts of detected errors, rerun with: -v
>> ==21917== All heap blocks were freed -- no leaks are possible.
>>
>> Unified diff of patch:
>> svn diff -x --unified
>> Index: libavformat/mov.c
>> ===================================================================
>> --- libavformat/mov.c   (revision 13158)
>> +++ libavformat/mov.c   (working copy)
>> @@ -1894,6 +1894,7 @@
>>          av_freep(&sc->drefs);
>>          if (sc->pb && sc->pb != s->pb)
>>              url_fclose(sc->pb);
>> +        av_freep(&sc);
>>      }
>>      if(mov->dv_demux){
>>          for(i=0; i<mov->dv_fctx->nb_streams; i++){
>>
>>
>>
>> (I know there's a two week gap between submitting a patch and ping back
> about a response, so feel free to ignore this until then :) ).
>
> In the event it would be helpful, attached is some really simple sample
> code (which I place in the public domain) that demonstrates the bug .  Just
> pass in a .mov file as the input.  Passing in a .flv or a .mp3 (those were
> the other formats I tested) don't leak (as expected).  Passing a .mov does
> leak before the fix, and doesn't after the fix.
>
> - Art
>
>
Lastly, I incorrect specified that to reproduce you just needed to open and
close the file.  That is not correct.

You need to also call av_find_stream_info() as well, so that libavformat
creates the AVStream object.  The sample code does the correct reproduction
of the problem.

Thanks,

- Art