Closed
Bug 874869
Opened 12 years ago
Closed 12 years ago
SEGV on mozilla::AudioChannelsDownMix
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox22 | --- | disabled |
firefox23 | - | disabled |
firefox24 | --- | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: attekett, Assigned: ehsan.akhgari)
References
Details
(4 keywords, Whiteboard: [adv-main24-])
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
Tested on:
OS: Ubuntu 12.04
Firefox: ASAN opt-build 24.0a1 from
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1369217427/
Repro-case:
<script>
var Context0= new AudioContext()
var Panner0=Context0.createPanner();
var BufferSource3=Context0.createBufferSource();
Panner0.channelCount=0;
BufferSource3.connect(Panner0);
BufferSource3.buffer=function(){
var length=1;
var Buffer=Context0.createBuffer(1,length,'44200');
var bufferData= Buffer.getChannelData(0);
for (var i = 0; i < length; ++i) { bufferData[i] = 255;};
return Buffer;
}();
</script>
ASAN-output:(opt-build)
=================================================================
==2893== ERROR: AddressSanitizer: SEGV on unknown address 0x7f5551b7fcdc (pc 0x7f514ce8e0a6 sp 0x7f5125511300 bp 0x7f51255113f0 T26)
AddressSanitizer can not provide additional info.
#0 0x7f514ce8e0a5 in mozilla::AudioChannelsDownMix(nsTArray<void const*> const&, float**, unsigned int, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioChannelFormat.cpp:175
#1 0x7f514ce91d57 in mozilla::AudioNodeStream::ObtainInputBlock(mozilla::AudioChunk&, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioNodeStream.cpp:347
#2 0x7f514ce92fe5 in mozilla::AudioNodeStream::ProduceOutput(long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioNodeStream.cpp:407
#3 0x7f514cf00c83 in mozilla::MediaStreamGraphImpl::ProduceDataForStreamsBlockByBlock(unsigned int, long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:937
#4 0x7f514cf12e05 in mozilla::(anonymous namespace)::MediaStreamGraphThreadRunnable::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:1163
#5 0x7f514f5a4212 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238
.
.
.
Updated•12 years ago
|
Assignee | ||
Comment 2•12 years ago
|
||
BTW, I think this is a safe crash, because we try to read memory from someArray[channelCount - 1] when channelCount is 0. I doubt that this is a security sensitive bug.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #752733 -
Flags: review?(roc)
Comment 4•12 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> BTW, I think this is a safe crash, because we try to read memory from
> someArray[channelCount - 1] when channelCount is 0. I doubt that this is a
> security sensitive bug.
Why should it be safe to read before someArray? Do you get the data back that is stored there?
Reporter | ||
Comment 5•12 years ago
|
||
There is second similar crash:
Repro-case:
<script>
var Context0= new AudioContext()
var BufferSource0=Context0.createBufferSource();
BufferSource0.connect(Context0.destination);
Context0.destination.channelCount=0;
BufferSource0.buffer=function(){
var length=73020;
var Buffer=Context0.createBuffer(1,length,Context0.sampleRate);
var bufferData= Buffer.getChannelData(0);
for (var i = 0; i < length; ++i) { bufferData[i] = Math.sin(i*(365))};
return Buffer;
}();
Context0.destination.channelInterpretation="discrete";
</script>
ASAN-report:
==20656== ERROR: AddressSanitizer: SEGV on unknown address 0x7f953d869cdc (pc 0x7f9138b77c56 sp 0x7f9111a5b4c0 bp 0x7f9111a5b5b0 T28)
AddressSanitizer can not provide additional info.
#0 0x7f9138b77c55 in mozilla::AudioChannelsUpMix(nsTArray<void const*>*, unsigned int, void const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioChannelFormat.cpp:91
#1 0x7f9138b86c54 in mozilla::AudioSegment::WriteTo(mozilla::AudioStream*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/AudioSegment.cpp:125
#2 0x7f9138be85fa in mozilla::MediaStreamGraphImpl::PlayAudio(mozilla::MediaStream*, long, long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:803
#3 0x7f9138beaf50 in mozilla::MediaStreamGraphImpl::RunThread() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/media/MediaStreamGraph.cpp:1031
.
.
.
btw, if you change Context0.destination.channelCount into small negative number, like -1, from this repro you will get oom.
ASAN-report:
==20539== WARNING: AddressSanitizer failed to allocate 0x0003fffffe10 bytes
out of memory: 0x00000003FFFFFE10 bytes requested
ASAN:SIGSEGV
=================================================================
==20539== ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7faab027b678 sp 0x7faa81e8f2f0 bp 0x7faa81e8f3a0 T25)
AddressSanitizer can not provide additional info.
#0 0x7faab027b677 in mozalloc_abort(char const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/memory/mozalloc/mozalloc_abort.cpp:30
#1 0x7faab027b3bc in moz_xmalloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/memory/mozalloc/mozalloc.cpp:56
.
.
.
Attachment #752733 -
Flags: review?(roc) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > BTW, I think this is a safe crash, because we try to read memory from
> > someArray[channelCount - 1] when channelCount is 0. I doubt that this is a
> > security sensitive bug.
>
> Why should it be safe to read before someArray? Do you get the data back
> that is stored there?
What I meant was that _if_ we crash here, it's going to be safe, since that would be the result of attempting to read an unmapped page.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Atte Kettunen from comment #5)
> There is second similar crash:
>
> Repro-case:
> <script>
> var Context0= new AudioContext()
> var BufferSource0=Context0.createBufferSource();
> BufferSource0.connect(Context0.destination);
>
> Context0.destination.channelCount=0;
> BufferSource0.buffer=function(){
> var length=73020;
> var Buffer=Context0.createBuffer(1,length,Context0.sampleRate);
> var bufferData= Buffer.getChannelData(0);
> for (var i = 0; i < length; ++i) { bufferData[i] = Math.sin(i*(365))};
> return Buffer;
> }();
> Context0.destination.channelInterpretation="discrete";
> </script>
Yeah, this bug happens when you set channelCount to 0. You could construct any number of scenarios which would trigger this bug in this exact way.
Fix landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2718954757e1
Assignee | ||
Comment 8•12 years ago
|
||
Needed a clobber <https://hg.mozilla.org/integration/mozilla-inbound/rev/e52c6f7200b3> because the build system sucks (see bug 874923.)
Comment 10•12 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
>
> What I meant was that _if_ we crash here, it's going to be safe, since that
> would be the result of attempting to read an unmapped page.
Okay, and what if we don't crash there? I don't see how this would lead to the conclusion that this is not a security problem or sec-low. The crash is not guaranteed, is it?
Comment 11•12 years ago
|
||
How do you guarantee that the memory before the array is unmapped?
What do you do with the object you do read should this not crash on an access violation?
Flags: needinfo?(ehsan)
Keywords: sec-low
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #11)
> How do you guarantee that the memory before the array is unmapped?
Comment 2 was poorly worded, and not well thought out. I retract that comment.
> What do you do with the object you do read should this not crash on an
> access violation?
So here's what happens. We use -1 as an index to the first array, and if that doesn't segfault, we use the read value + some non-zero offset from another array <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#173>. If none of these segfault, we start filling an array using the values from the invalid pointer that we have read + a non-negative offset <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#185> and <http://mxr.mozilla.org/mozilla-central/source/content/media/AudioChannelFormat.cpp#190>, and if that doesn't segfault either, we're probably overwriting memory which doesn't belong to us. But the things we write to the memory are floating point multiplications based on values read from garbage memory as well (plus the values coming from the previous node in the graph). In theory, it's possible to craft everything in a way that you end up writing what you want to the memory address that you want, but it definitely requires some heroics being put into it. Which is why I suggested in comment 2 that this is probably not a security bug, but I guess that theoretical possibility still exists.
Flags: needinfo?(ehsan)
Comment 13•12 years ago
|
||
Backed out for intermittent Windows leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8402ad9f66
https://tbpl.mozilla.org/php/getParsedLog.php?id=23274877&tree=Mozilla-Inbound
Comment 14•12 years ago
|
||
That's quite odd. I wonder if there is some bindings-related leak, as I can't see how this patch would cause leaks by itself. Unless the test case happens to trigger leaks.
Assignee | ||
Comment 15•12 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #14)
> That's quite odd. I wonder if there is some bindings-related leak, as I
> can't see how this patch would cause leaks by itself. Unless the test case
> happens to trigger leaks.
The testcase is in the crashtest suite...
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
So I pushed the patch landed on inbound and retriggered mochitest-1 runs on Windows on it, and the leak did not happen again: https://tbpl.mozilla.org/?tree=Try&rev=bc1cd1f75c78
This shows that the leak was intermittent in fact, and the patch was prematurely backed out. So I relanded it: https://hg.mozilla.org/integration/mozilla-inbound/rev/96b964d758c8
Comment 18•12 years ago
|
||
Updated•12 years ago
|
Target Milestone: --- → mozilla24
Updated•12 years ago
|
status-b2g18:
--- → unaffected
status-firefox22:
--- → disabled
status-firefox23:
--- → affected
status-firefox-esr17:
--- → unaffected
Updated•12 years ago
|
Keywords: csec-bounds,
sec-high
Updated•12 years ago
|
tracking-firefox23:
--- → +
Assignee | ||
Comment 19•11 years ago
|
||
Mass moving Web Audio bugs to the Web Audio component. Filter on duckityduck.
Component: Video/Audio → Web Audio
Updated•11 years ago
|
Flags: sec-bounty?
Updated•11 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 20•11 years ago
|
||
No longer tracking for FF23
Updated•11 years ago
|
Attachment #763835 -
Attachment description: Bounty Awarded $3000 → Bounty Awarded $3000 [paid] 6/19/13
Updated•11 years ago
|
Whiteboard: [adv-main24-]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•