1066943 - Large OOM in mozilla::WebMReader::DecodeAudioPacket

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[David Vo [:auscompgeek]]

A user on freenode has crafted a webm file that consistently causes an OOM crash in mozilla::WebMReader::DecodeAudioPacket.

Regression window (release):
- Last working release: 27.0.1
- First busted release: 28.0

The 24 ESR branch isn't affected.

Tested:
- Doesn't crash: 24.7, 24.8, 26.0, 27.0
- Crashes: 28.0, 29.0.1, 31.0, 32.0, 32.0.1

Crash reports:

bp-76739c03-7909-4ba7-bbc7-f25c42140913
bp-47ec6919-51b0-4a98-af16-db71c2140913
bp-3a806b64-46a0-48af-be45-734472140905

https://crash-stats.mozilla.com/report/list?signature=OOM+%7C+large+%7C+mozalloc_abort%28char+const%2A+const%29+%7C+mozalloc_handle_oom%28unsigned+int%29+%7C+moz_xmalloc+%7C+mozilla%3A%3AWebMReader%3A%3ADecodeAudioPacket%28nestegg_packet%2A%2C+__int64%29&product=Firefox&query_type=contains&range_unit=days&process_type=any&hang_type=any&date=2014-09-13+08%3A00%3A00&range_value=30#reports

Crashing thread (on my Ubuntu 14.04 x86_64):

0 	libmozalloc.so 	mozalloc_abort(char const*) 	memory/mozalloc/mozalloc_abort.cpp
1 	libmozalloc.so 	mozalloc_handle_oom(unsigned long) 	memory/mozalloc/mozalloc_oom.cpp
2 	libmozalloc.so 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp
3 	libxul.so 	mozilla::WebMReader::DecodeAudioPacket(nestegg_packet*, long) 	obj-firefox/dist/include/mozilla/mozalloc.h
4 	libxul.so 	mozilla::WebMReader::DecodeAudioData() 	content/media/webm/WebMReader.cpp
5 	libxul.so 	mozilla::MediaDecoderReader::RequestAudioData() 	content/media/MediaDecoderReader.cpp
6 	libxul.so 	mozilla::MediaDecoderStateMachine::DecodeAudio() 	content/media/MediaDecoderStateMachine.cpp
7 	libxul.so 	nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() 	obj-firefox/dist/include/nsThreadUtils.h
8 	libxul.so 	mozilla::MediaTaskQueue::Runner::Run() 	content/media/MediaTaskQueue.cpp
9 	libxul.so 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp
10 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
11 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
12 	libxul.so 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
13 	libxul.so 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
14 	libxul.so 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp
15 	libnspr4.so 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c

Comment 1

[Matthew Gregan [:kinetik]]

(In reply to auscompgeek from comment #0)
> Tested:
> - Doesn't crash: 24.7, 24.8, 26.0, 27.0
> - Crashes: 28.0, 29.0.1, 31.0, 32.0, 32.0.1

Opus support was added in 28 (bug 938686) which involved a bunch of changes to WebMReader.  That's probably a good place to start looking.

Indeed, we crash at:

    nsAutoArrayPtr<AudioDataValue> trimBuffer(new AudioDataValue[samples]);

samples == -1105026944
keepFrames == 1594970176
discardPadding == 56250000
frames == 2880

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: 0001-Bug-1066943-Fix-Opus-discard-handling.patch

| + Block group at 67568
|  + Block (track number 2, 1 frame(s), timecode 1.380s = 00:00:01.380) at 67577
|   + Frame with size 1166
|  + Discard padding: 56.250ms (56250000ns) at 68750
| + [I frame for track 2, timecode 1380]

The core problem problem is:

-        CheckedInt64 discardFrames = UsecsToFrames(discardPadding * NS_PER_USEC, rate);
+        CheckedInt64 discardFrames = UsecsToFrames(discardPadding / NS_PER_USEC, rate);

Causing discardFrames to be 2700000000 rather than 2700.

It looks like it's also possible (but not happening in this case) for the discard to be larger than the decoded frames, which was handled by discarding the entire block, except it could underflow is discard was large enough.  My patch addresses that as well.  I couldn't find anything that clearly indicated a discard larger than the block was valid or invalid, so this patch retains the existing behaviour.

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8491224 [details] [diff] [review]
0001-Bug-1066943-Fix-Opus-discard-handling.patch

Review of attachment 8491224 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: content/media/webm/WebMReader.cpp
@@ +696,3 @@
>            return true;
>          }
> +        int32_t keepFrames = frames - discardFrames.value();

You need to handle discardFrames < 0 as well. You can just reject such files in this patch.

@@ +699,5 @@
> +        int32_t samples = keepFrames * channels;
> +        nsAutoArrayPtr<AudioDataValue> trimBuffer(new AudioDataValue[samples]);
> +        for (int32_t i = 0; i < samples; i++) {
> +          trimBuffer[i] = buffer[i];
> +        }

PodCopy()?

Comment 4

[Ralph Giles (:rillian)]

(In reply to Matthew Gregan [:kinetik] from comment #2)

>                                            I couldn't find anything that
> clearly indicated a discard larger than the block was valid or invalid, so
> this patch retains the existing behaviour.

Good point, we need to clean that up. I would be happy so see another patch rejecting files where this happens. In general, our policy with Opus has been to be as strict as possible in rejecting incorrect files to encourage precision in encoder implementations. We can relax later if forced to by popular files in the wild, but going the other direction is harder.

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Ralph Giles (:rillian) from comment #3)
> You need to handle discardFrames < 0 as well. You can just reject such files
> in this patch.

It's handled (by ignoring it) by all of the discard handling being wrapped with:

    if (discardPadding > 0) {

discardFrames can't become negative if discardPadding is positive.

I'd prefer to leave rejecting the file to bug 1069660.

> @@ +699,5 @@
> > +        int32_t samples = keepFrames * channels;
> > +        nsAutoArrayPtr<AudioDataValue> trimBuffer(new AudioDataValue[samples]);
> > +        for (int32_t i = 0; i < samples; i++) {
> > +          trimBuffer[i] = buffer[i];
> > +        }
> 
> PodCopy()?

Sure.  We can probably reduce the amount of copying here too, all we really need to do is adjust the start and end point of the buffer array.  :-/

Comment 6

[Ralph Giles (:rillian)]

>     if (discardPadding > 0) {

Right. Sorry I missed that.

Comment 7

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/777897528b20
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b97ca88f5f

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/777897528b20
https://hg.mozilla.org/mozilla-central/rev/79b97ca88f5f