Closed Bug 877039 Opened 11 years ago Closed 11 years ago

WebAudio OOM crash [@AllocateAudioBlock]

Categories

(Core :: Web Audio, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- disabled
firefox23 --- disabled
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: posidron, Assigned: ehsan.akhgari)

References

Details

(Keywords: crash, csectype-dos, testcase, Whiteboard: [adv-main24-])

Attachments

(4 files)

Attached file callstack
void
AllocateAudioBlock(uint32_t aChannelCount, AudioChunk* aChunk)
{
  // XXX for SIMD purposes we should do something here to make sure the
  // channel buffers are 16-byte aligned.
  nsRefPtr<SharedBuffer> buffer =
    SharedBuffer::Create(WEBAUDIO_BLOCK_SIZE*aChannelCount*sizeof(float));
[...]


Tested with m-i changeset: 133184:944f1e046ea2
Attached file callstack-abort.txt
I still had no time to figured out how to set those values externally and to provide a testcase, though I am getting fairly regularly those two type of crashes. 

We have marked this preventively as s-s is because of the multiplication for the size of the allocated buffer. .. It needs further investigation.
Attached file testcase
Inside the testcase different values for channelCount produce different results.


o1.destination.channelCount = 268435456;

Produces: Trying to allocate an infallible array that's too big


o1.destination.channelCount = 268435455;

Produces: 
==2538==WARNING: AddressSanitizer failed to allocate 0x0003fffffe10 bytes
out of memory: 0x00000003FFFFFE10 bytes requested


o1.destination.channelCount = -1;

Produces:
==2571==WARNING: AddressSanitizer failed to allocate 0x0003fffffe10 bytes
out of memory: 0x00000003FFFFFE10 bytes requested


o1.destination.channelCount = -0xfffffff;
Produces: Trying to allocate an infallible array that's too big
Keywords: testcase
That seems like an insane number of channels. Was just watching an episode of ST:TNG with my family and even Data said he could only process a few tens of thousands of simultaneous audio streams. Can we cap this at a level that prevents the danger of integer overflow?
IIRC roc was opposed to having an arbitrary low number of channels before.  FWIW WebKit/Blink impose a limit of 32 channels maximum, and I personally would like if we did that.  Otherwise the proper way forward here is to do fallible allocations and do something sane for allocation failures (such as outputting silence).  I could do either, but let's see what roc thinks first before I write the patch.
Flags: needinfo?(roc)
I don't really care. Millions of channels is clearly stupid. We should set the limit to something that prevents integer overflows. If 32 helps, let's do it, but I would have thought your other patch that used 10000 would be fine.
Flags: needinfo?(roc)
Oh wait.  I was testing this with the wrong build.  :(  With a build that has the fix to bug 876118, I cannot repro, so this seems like a dupe of bug 876118, IINM.

The changeset mentioned in comment 0 (133184:944f1e046ea2) doesn't exist, as far as I can tell... Christoph, can you really reproduce this with the fix to bug 876118 applied?  What *should* happen is that setting channelCount to these large values must throw...
Flags: needinfo?(cdiehl)
Okay, this is fixed now.

Tested with http://hg.mozilla.org/integration/mozilla-inbound/rev/d7c6d6061ab5
Flags: needinfo?(cdiehl)
Actually thinking about this more, I'd still like to adjust the maximum channel count value to 32, since currently each chunk that we allocate can be nearly 5MB large, and especially on mobile that provides a very easy OOM path...
Attached patch Patch (v1)Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #755912 - Flags: review?(roc)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> Actually thinking about this more, I'd still like to adjust the maximum
> channel count value to 32, since currently each chunk that we allocate can
> be nearly 5MB large, and especially on mobile that provides a very easy OOM
> path...

I don't think this matters. There are many easy OOM paths.
I'm actually limiting the number of channels for the OfflineAudioContext to 32 in bug 865244 (webkit/blink limit is 10). It would make sense to limit it for the AudioContext as well.

fwiw, most audio backends cannot play more than 32 channels anyway (that is, even when using another software than Firefox, the file could not be played without downmixing or channel dropping).
https://hg.mozilla.org/mozilla-central/rev/fe6ca42a5b6e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Whiteboard: [adv-main24-]
Keywords: csec-dos
See Also: → 990794
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: