Fix valgrind and asan memory leaks and crashes.

Fix valgrind and asan memory leaks and crashes.

Lots of fixes and patch cleanup.  All these fixes are necessary to get
ffmpeg_regression_tests passing w/o crashes on ia32/x64 platforms.

Both utils.c changes are likely incompatible with all codecs, the
codec->close() and iformat->read_close() methods for desired codecs must
be sanitized to ensure they are not double-free'ing.

BUG=none
TEST=ffmpeg_regression_tests

[DaleCurtis, 2012-02-08 23:47:58]

PTAL. Potential fixes for 14 valgrind issues. See valgrind.txt for the errors.

[rbultje1, 2012-02-08 23:51:37]

On 2012/02/08 23:47:58, DaleCurtis wrote:
> PTAL. Potential fixes for 14 valgrind issues. See valgrind.txt for the errors.

[rbultje1, 2012-02-08 23:51:56]

Oops, wrong button. Here's the real comments.

https://chromiumcodereview.appspot.com/9373002/diff/1/libavcodec/vp3.c
File libavcodec/vp3.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/1/libavcodec/vp3.c#newcod...
libavcodec/vp3.c:1634: memset(s->dct_tokens_base, 0, 64*s->fragment_count *
sizeof(*s->dct_tokens_base));
Use av_mallocz() instead of av_malloc() to zero it automatically.

https://chromiumcodereview.appspot.com/9373002/diff/1/libavformat/utils.c
File libavformat/utils.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/1/libavformat/utils.c#new...
libavformat/utils.c:2710: av_freep(&sc->keyframes);
av_freep() does the if() internally, so no need for the if() here.

[scherkus (not reviewing), 2012-02-09 00:38:14]

deferring to rbultje :)

[DaleCurtis, 2012-02-09 04:03:27]

https://chromiumcodereview.appspot.com/9373002/diff/1/libavcodec/vp3.c
File libavcodec/vp3.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/1/libavcodec/vp3.c#newcod...
libavcodec/vp3.c:1634: memset(s->dct_tokens_base, 0, 64*s->fragment_count *
sizeof(*s->dct_tokens_base));
On 2012/02/08 23:51:56, rbultje1 wrote:
> Use av_mallocz() instead of av_malloc() to zero it automatically.

Done.

https://chromiumcodereview.appspot.com/9373002/diff/1/libavformat/utils.c
File libavformat/utils.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/1/libavformat/utils.c#new...
libavformat/utils.c:2710: av_freep(&sc->keyframes);
On 2012/02/08 23:51:56, rbultje1 wrote:
> av_freep() does the if() internally, so no need for the if() here.

Done.

[DaleCurtis, 2012-02-09 22:43:26]

Now with some crash fixes. Again probably not completely right, but prevents
crashes.

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c
File libavcodec/pthread.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c...
libavcodec/pthread.c:613: if (fctx->next_decoding >= avctx->thread_count) return
err;
Proper fix is likely related to acolwell's threading fixes, this prevents the
crash for now.

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/oggparse...
File libavformat/oggparsetheora.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/oggparse...
libavformat/oggparsetheora.c:133: if (!thp) return NULL;
Crash fix for http://tskir-html5.kir.corp.google.com/security/wav.711.ogv

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt
File valgrind.txt (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode504
valgrind.txt:504: //----------- security/100464.webm crash.
Fixed in pthread.c

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode553
valgrind.txt:553: /// --------- security/wav.711.ogv crash.
Fixed in oggparsetheora.c

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode582
valgrind.txt:582: /// --------- noticed after fix for above crash:
Couldn't find a way to stop this leak, it's a small one though. Unsure if caused
by my crash fix or just tangential.

[rbultje1, 2012-02-09 23:27:57]

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c
File libavformat/utils.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c#...
libavformat/utils.c:2707: MOVStreamContext *sc = st->priv_data;
I'm actually wondering if we shouldn't check somehow that we're demuxing a .mov
file, and not something else that just happens to set st->priv_data. Or
alternatively, maybe free this in the mov demuxer. I'm pretty sure this could
crash many other file types that set st->priv_data.

[rbultje1, 2012-02-09 23:28:45]

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c
File libavcodec/pthread.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c...
libavcodec/pthread.c:613: if (fctx->next_decoding >= avctx->thread_count) return
err;
On 2012/02/09 22:43:26, DaleCurtis wrote:
> Proper fix is likely related to acolwell's threading fixes, this prevents the
> crash for now.

This looks really evil, I'm OK with the workaround but where do I find this
file? I'd like to have a look at it to see if there's a better fix.

[DaleCurtis, 2012-02-10 00:04:01]

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c
File libavcodec/pthread.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavcodec/pthread.c...
libavcodec/pthread.c:613: if (fctx->next_decoding >= avctx->thread_count) return
err;
On 2012/02/09 23:28:46, rbultje1 wrote:
> On 2012/02/09 22:43:26, DaleCurtis wrote:
> > Proper fix is likely related to acolwell's threading fixes, this prevents
the
> > crash for now.
> 
> This looks really evil, I'm OK with the workaround but where do I find this
> file? I'd like to have a look at it to see if there's a better fix.

I've commented on valgrind.txt to link each test file with each report.

http://xorax.sea/ffmpeg-security/111342.ogm

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c
File libavformat/utils.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c#...
libavformat/utils.c:2707: MOVStreamContext *sc = st->priv_data;
On 2012/02/09 23:27:58, rbultje1 wrote:
> I'm actually wondering if we shouldn't check somehow that we're demuxing a
.mov
> file, and not something else that just happens to set st->priv_data. Or
> alternatively, maybe free this in the mov demuxer. I'm pretty sure this could
> crash many other file types that set st->priv_data.

I'll take another look and see if there's somewhere else I can put this code.
Really we only need to fix the crashes before I push, we can investigate better
solutions for the memory leaks.

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt
File valgrind.txt (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode3
valgrind.txt:3: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode38
valgrind.txt:38: 15:44:14 memcheck_analyze.py [ERROR] Command:
http://xorax.sea/ffmpeg-security/big_dims.ogv

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode63
valgrind.txt:63: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode99
valgrind.txt:99: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/112670.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode133
valgrind.txt:133: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode173
valgrind.txt:173: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode209
valgrind.txt:209: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/112670.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode245
valgrind.txt:245: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode282
valgrind.txt:282: 1,196 bytes in 1 blocks are definitely lost in loss record 567
of 604
http://xorax.sea/ffmpeg-security/112670.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode313
valgrind.txt:313: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode353
valgrind.txt:353: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode389
valgrind.txt:389: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/h264.705767.mp4
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode429
valgrind.txt:429: Leak_DefinitelyLost
http://xorax.sea/ffmpeg-security/smclockmp4aac_1_0.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode472
valgrind.txt:472: 278,601 bytes in 1 blocks are definitely lost in loss record
601 of 604
http://xorax.sea/ffmpeg-security/112670.mp4

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode504
valgrind.txt:504: //----------- security/100464.webm crash.
On 2012/02/09 22:43:26, DaleCurtis wrote:
> Fixed in pthread.c

http://xorax.sea/ffmpeg-security/100464.webm

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode530
valgrind.txt:530: 13:20:20 memcheck_analyze.py [ERROR] Command:
http://xorax.sea/ffmpeg-security/111342.ogm

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode553
valgrind.txt:553: /// --------- security/wav.711.ogv crash.
On 2012/02/09 22:43:26, DaleCurtis wrote:
> Fixed in oggparsetheora.c

http://xorax.sea/ffmpeg-security/wav.711.ogv

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode582
valgrind.txt:582: /// --------- noticed after fix for above crash:
On 2012/02/09 22:43:26, DaleCurtis wrote:
> Couldn't find a way to stop this leak, it's a small one though. Unsure if
caused
> by my crash fix or just tangential.

http://xorax.sea/ffmpeg-security/wav.711.ogv

[DaleCurtis, 2012-02-10 23:04:35]

PTAL. This now fixes all known issues on Linux x64.

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c
File libavformat/utils.c (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/libavformat/utils.c#...
libavformat/utils.c:2707: MOVStreamContext *sc = st->priv_data;
Done. Moved into read_close() in mov.c and added a read_close() call on
read_header() failure.

On 2012/02/10 00:04:01, DaleCurtis wrote:
> On 2012/02/09 23:27:58, rbultje1 wrote:
> > I'm actually wondering if we shouldn't check somehow that we're demuxing a
> .mov
> > file, and not something else that just happens to set st->priv_data. Or
> > alternatively, maybe free this in the mov demuxer. I'm pretty sure this
could
> > crash many other file types that set st->priv_data.
> 
> I'll take another look and see if there's somewhere else I can put this code.
> Really we only need to fix the crashes before I push, we can investigate
better
> solutions for the memory leaks.

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt
File valgrind.txt (right):

https://chromiumcodereview.appspot.com/9373002/diff/5010/valgrind.txt#newcode582
valgrind.txt:582: /// --------- noticed after fix for above crash:
New patch set has a fix for this in ogg_read_close.

On 2012/02/10 00:04:01, DaleCurtis wrote:
> On 2012/02/09 22:43:26, DaleCurtis wrote:
> > Couldn't find a way to stop this leak, it's a small one though. Unsure if
> caused
> > by my crash fix or just tangential.
> 
> http://xorax.sea/ffmpeg-security/wav.711.ogv

[rbultje1, 2012-02-13 21:54:54]

lgtm

LGTM.