Closed Bug 1181718 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: tsmith, Assigned: gerald)

References

Details

(Keywords: csectype-oom)

Attachments

(4 files, 2 obsolete files)

Attached video test_case.mp4
This test case triggers an OOm on Asan builds and seems to stop and pin the CPU on normal builds.
Attached file test.log (obsolete) —
Component: Audio/Video → Audio/Video: Playback
Attached file call_stack.txt
Attachment #8631183 - Attachment is obsolete: true
Interested in this one, Gerald.
Flags: needinfo?(gsquelart)
Priority: -- → P2
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)
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+
(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+
https://hg.mozilla.org/mozilla-central/rev/a3745c645a77
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.