Large OOM in mozilla::WebMReader::DecodeAudioPacket

RESOLVED FIXED in mozilla35

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: auscompgeek, Assigned: kinetik)

Tracking

({crash})

unspecified
mozilla35
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
Summary: crash in OOM | large | mozalloc_abort(char const*) | mozalloc_handle_oom(unsigned long) | moz_xmalloc | mozilla::WebMReader::DecodeAudioPacket(nestegg_packet*, long) → Large OOM in mozilla::WebMReader::DecodeAudioPacket
(Assignee)

Comment 1

5 years ago
(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
(Assignee)

Updated

5 years ago
Blocks: 938686
(Assignee)

Updated

5 years ago
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
| + 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.
Attachment #8491224 - Flags: review?(giles)
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()?
Attachment #8491224 - Flags: review?(giles) → review+
(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.
(Assignee)

Comment 5

5 years ago
(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.  :-/
>     if (discardPadding > 0) {

Right. Sorry I missed that.
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/777897528b20
https://hg.mozilla.org/mozilla-central/rev/79b97ca88f5f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.