Closed
Bug 1181718
Opened 9 years ago
Closed 8 years ago
mozilla::BlankAudioDataCreator::Create(long, long, long) OOM crash on ASan builds
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: tsmith, Assigned: mozbugz)
References
Details
(Keywords: csectype-oom)
Attachments
(4 files, 2 obsolete files)
This test case triggers an OOm on Asan builds and seems to stop and pin the CPU on normal builds.
Reporter | ||
Comment 1•9 years ago
|
||
Reporter | ||
Comment 2•9 years ago
|
||
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Reporter | ||
Comment 3•9 years ago
|
||
Attachment #8631183 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 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•8 years ago
|
||
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 7•8 years ago
|
||
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•8 years ago
|
||
(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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1724dc8506b7
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3745c645a77
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3745c645a77
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•