mozilla::BlankAudioDataCreator::Create(long, long, long) OOM crash on ASan builds

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: gerald)

Tracking

({csectype-oom})

unspecified
mozilla45
csectype-oom
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8631182 [details]
test_case.mp4

This test case triggers an OOm on Asan builds and seems to stop and pin the CPU on normal builds.
(Reporter)

Comment 1

2 years ago
Created attachment 8631183 [details]
test.log
(Reporter)

Comment 2

2 years ago
Created attachment 8641942 [details]
null_deref_call_stack.txt
Component: Audio/Video → Audio/Video: Playback
(Reporter)

Comment 3

2 years ago
Created attachment 8683868 [details]
call_stack.txt
Attachment #8631183 - Attachment is obsolete: true
Interested in this one, Gerald.
Flags: needinfo?(gsquelart)
Priority: -- → P2
(Assignee)

Comment 5

2 years ago
From what I can see, the audio samples at the start of the file contain around 1000 frames, for a duration of around 0.02s.
But the last six audio samples starting at DTS=0.720204s contain 16,906,121 frames each! (~383s)
Either the samples themselves are fuzzed, or something before lead the demuxer and/or decoder to read the wrong data.

The ASAN stack trace points at the allocation in the blank decoder, of frames*channels*sizeof(float) ~ 129MB. And it could happen up to 6 times, for 774MB in total.

In other non-ASAN circumstances these allocations could work, in which case I can imagine the CPU being busy filling them with sine-wave data, and then playing all these sample for 383s each.


And here's the main "can of worms" issue: There are many paths that this data could take depending on the platform, some of which go to external decoders, which may have unknown effects when given these samples.
For example, Mac OS X's VideoToolbox (video?) decoder returns no data and then refuses more input, which stops playback with an error -- probably a good thing.

Anthony was suggesting implementing sanitizers before handing the data over to platform decoders. Maybe this would help here.


In the meantime, the only thing I think I can do is to make the allocations fallible in the blank decoder.

Other suggestions?
Assignee: nobody → gsquelart
Flags: needinfo?(gsquelart)
(Assignee)

Comment 6

2 years ago
Created attachment 8691758 [details] [diff] [review]
1181718-fallible-blank-audio.patch

Fallible allocation of blank audio data.

This is mostly to guard against allocating crazy amounts of memory due to
invalid data -- in this case a sample with an excessive number of frames.
Attachment #8691758 - Flags: review?(jyavenard)
Comment on attachment 8691758 [details] [diff] [review]
1181718-fallible-blank-audio.patch

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

::: dom/media/platforms/agnostic/BlankDecoderModule.cpp
@@ +186,5 @@
>          frames.value() > (UINT32_MAX / mChannelCount)) {
>        return nullptr;
>      }
> +    UniquePtr<AudioDataValue[]> samples(
> +      new (fallible) AudioDataValue[frames.value() * mChannelCount]);

don't you have to include "fallible.h"?

it will likely compile thanks to unified builds.

Personally, I would use a nsTArray as backend and use the new fallible option (don't need to use FallibleTArray anymore):
e.g.
nsTArray<AudioDataValue> samples;
if (!samples.SetLength(frames.value() * mChannelCount, fallible)) { return nullptr }

A bit more mozillian.
Attachment #8691758 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 8

2 years ago
Created attachment 8692710 [details] [diff] [review]
1181718-fallible-blank-audio.patch v2

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Comment on attachment 8691758 [details] [diff] [review]
> 1181718-fallible-blank-audio.patch
> 
> Review of attachment 8691758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/agnostic/BlankDecoderModule.cpp
> @@ +189,2 @@
> > +    UniquePtr<AudioDataValue[]> samples(
> > +      new (fallible) AudioDataValue[frames.value() * mChannelCount]);
> 
> don't you have to include "fallible.h"?
> 
> it will likely compile thanks to unified builds.

Word from the expert (I asked about "mozalloc.h", which defines 'operator new' and includes fallible.h) :
<glandium> as long as you #include a STL header, you don't need to include mozalloc.h

So I've included "mozalloc.h", and sorted #include lines.

> Personally, I would use a nsTArray as backend and use the new fallible
> option (don't need to use FallibleTArray anymore):
> e.g.
> nsTArray<AudioDataValue> samples;
> if (!samples.SetLength(frames.value() * mChannelCount, fallible)) { return
> nullptr }
> 
> A bit more mozillian.

Unfortunately, 'samples' is then passed to a function that expects a Move'd UniquePtr<AudioDataValue[]>.

Carrying r=jya.
Attachment #8691758 - Attachment is obsolete: true
Attachment #8692710 - Flags: review+
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1724dc8506b7
Keywords: checkin-needed

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3745c645a77
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3745c645a77
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Updated

2 years ago
Blocks: 1229965
You need to log in before you can comment on or make changes to this bug.