944851 - Array out-of-bounds [@ mozilla::dom::AudioParamTimeline::AudioNodeInputValue]

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: i < Length() (invalid array index), at nsTArray.h:887

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Karl Tomlinson (:karlt back Feb 10)]

This is an invalid read of an uninitialized AudioChunk from an nsAutoTArray in an AudioNodeStream.  AudioChunk contains a ThreadSharedObject, which has virtual functions, but these are not executed, so I don't think arbitrary code execution is possible.  However, the chunk contains uninitialized pointers to 512 block chunks of memory that could be read.  It may be possible, though likely difficult, for content to influence those pointers.

Comment 3

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: time-varying testcase

We shouldn't be locking to include data from the audio thread here, and trying
to use the current value from the audio thread will only introduce races.  I
guess we could get the audio thread to send its latest value to the main
thread, but that is expensive - I don't want to do that.

This testcase demonstrates that Blink fails to even try to generate reasonable
results if anything is time-varying.
http://src.chromium.org/viewvc/blink/trunk/Source/modules/webaudio/AudioParam.cpp?revision=159093#l116

Gecko gives the expected result:

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0.9565200209617615, phase = -1.5707964897155762

Blink produces incorrect results, which may be dependent on uninitialized
values:

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0, phase = 0

Defaults:
magnitude = 0.9565200209617615, phase = -1.5707964897155762
With parameter timeline:
magnitude = 0, phase = -3.1415927410125732

Comment 4

[Karl Tomlinson (:karlt back Feb 10)]

Attachment: don't consider AudioNode input when getting AudioParam values on the main thread

Comment 5

[Karl Tomlinson (:karlt back Feb 10)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4da19ec3e3b

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f4da19ec3e3b

Comment 7

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 866434
User impact if declined: moderate security risk
Testing completed (on m-c, etc.): on m-c.
Manually tested that the code behaves as expected.
Risk to taking this patch (and alternatives if risky): 
Small.  The common paths have no change in behavior.  There is a behavior change in an unusual situation.  I don't expect this to cause any problems because Chrome does not produce reasonable results in this situation.
String or IDL/UUID changes made by this patch: none.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/07df8373cf97
https://hg.mozilla.org/releases/mozilla-beta/rev/3d79e4c4b354

Is this something we need on b2g26 (v1.2)?

Comment 9

[Karl Tomlinson (:karlt back Feb 10)]

Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

Please see comment 7.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> Is this something we need on b2g26 (v1.2)?

Yes, it is, thank you.

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in FF27, 2013-11-22.
Verified fixed in FF27, FF28 and FF29, 2014-01-14.

Comment 11

[Preeti Raghunath(:Preeti)]

Comment on attachment 8351280 [details] [diff] [review]
don't consider AudioNode input when getting AudioParam values on the main thread

plus on approval‑mozilla‑b2g26: for sec-moderate

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/86deef7b5eb7

Comment 13

[Karl Tomlinson (:karlt back Feb 10)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd6da0400ff

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbd6da0400ff