350 bytes, text/html
2.92 KB, patch
|Details | Diff | Splinter Review|
4.43 KB, patch
|Details | Diff | Splinter Review|
FF Version: https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1416229949/firefox-36.0a1.en-US.linux-x86_64-asan.tar.bz2 Assertion + Stacktrace: Assertion failure: aIndex < Length() (invalid array index), at ../../../../dist/include/nsTArray.h:946 #0 0x00007fffe8459974 in nsTArray_Impl<mozilla::AudioChunk, nsTArrayInfallibleAllocator>::ElementAt (this=<optimized out>, aIndex=<optimized out>) at ../../../../dist/include/nsTArray.h:946 #1 0x00007fffea3e10bc in mozilla::dom::AudioParamTimeline::AudioNodeInputValue (this=<optimized out>, aCounter=0) at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/AudioParam.cpp:135 #2 0x00007fffea3ecbed in mozilla::dom::AudioParamTimeline::GetValueAtTime<long> (this=0x6110005b4448, aTime=<optimized out>, aCounter=0) at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/AudioParamTimeline.h:93 #3 0x00007fffea417580 in mozilla::dom::DelayNodeEngine::UpdateOutputBlock ( this=0x6110005b4400, aOutput=<optimized out>, minDelay=<optimized out>) at /builds/slave/m-cen-l64-asan-d-0000000000000/build/dom/media/webaudio/DelayNode.cpp:142 ... With a release build of firefox the testcase gives a invalid read at 0x5a5a5a62 looks like poisoned memory.
It crashes FF33 on Win 7 too.
FF36 CR (with e10s): https://crash-stats.mozilla.com/report/index/1c73a631-ff20-40af-b5df-899522141117 Regression range: good=2014-07-18 bad=2014-07-19 http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=99f694d1b50c&tochange=35f3fa435d2c Suspected nug: Karl Tomlinson — b=932400 change stream ordering to get feedback DelayNode output before supplying input r=roc Previously downstream nodes from DelayNodes in cycles sometimes received stale output from the previous MSG iteration. Also, if two cycles share a common path, they will now *both* be treated as cycles, either by muting or by enforcing minimum delay. Previously, marking one cycle first could prevent detection of other cycles in the same SCC.
This permits a read of 512kB block of memory with an uninitialized pointer.
Assignee: nobody → karlt
AudioNodeEngine::mOutputCount is const so the array length can be set early. There is a deeper issue here that I can file once this is fixed on branches: A delay node, as currently spec'd can't produce output before it has input on its parameter, to know how much delay to apply. Either the delay node algorithm could be changed to apply the delay to the input instead of to the output, or cycles involving the delay node parameter could be muted.
Attachment #8524276 - Flags: review?(padenot)
Given the pointer is near nsTArrayHeader::sEmptyHdr, I doubt content can control the read.
Attachment #8524276 - Flags: review?(padenot) → review+
Overall stability of 34 is in an acceptable range. Given that this is sec-moderate and we don't need to take further stability fixes, I'm marking this as wontfix for 34.
The previous patch failed to compile in non-unified builds due to mEngine->OutputCount() requiring the definition of nsAudioNodeEngine.
Attachment #8526403 - Flags: review?(padenot)
Rebased attachment 8524276 [details] [diff] [review] on top of attachment 8526403 [details] [diff] [review].
Attachment #8524276 - Attachment is obsolete: true
Attachment #8526403 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/bbe24bcf277e https://hg.mozilla.org/mozilla-central/rev/874e07bec1cc Please nominate this for Aurora and b2g34 approval when you get a chance.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8526403 [details] [diff] [review] uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h Please consider this an approval request for both patches here. Approval Request Comment [Feature/regressing bug #]: The particular testcase here didn't trigger the problem until changes from bug 932400, but I expect slightly different web audio graphs would produce the same crash even in earlier releases, as far back as enabling webaudio for 25. [User impact if declined]: sec-moderate crash. [Describe test coverage new/current, TBPL]: This particular crash is tested only against the testcase in this bug. Delaying landing this test until the security flaw is patched on release branches. The code is widely tested in automated tests (but they don't cover this case). [Risks and why]: Some risks associated with array manipulation, but existing testing provides assurances. [String/UUID change made/needed]: None.
Comment on attachment 8526403 [details] [diff] [review] uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h The graph ordering was changed in bug 932400, which means this testcase doesn't affect earlier versions and can't be used to verify, but the underlying bug was still present before changes from bug 932400.
Comment on attachment 8526403 [details] [diff] [review] uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h we merged in between. cf comment #12 for uplift the request.
Attachment #8526403 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8526403 [details] [diff] [review] uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h Approving for Beta uplift, wontfix for ESR31 as it doesn't meet the landing criteria nor does it appear to be a highly requested fix from that user base.
Attachment #8526403 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8526403 [details] [diff] [review] uninline AudioNodeStream constructor to avoid requiring AudioNodeEngine.h Approving on 2.1 as this is the release version in flight where we could avoid this bug by this uplift. For other branches we are only uplifting sec-crit's alone at this time and this will be a wontfix there.
Attachment #8526403 - Flags: approval-mozilla-b2g34?
Attachment #8526403 - Flags: approval-mozilla-b2g34+
Attachment #8526403 - Flags: approval-mozilla-b2g32?
Attachment #8526403 - Flags: approval-mozilla-b2g32-
Attachment #8526403 - Flags: approval-mozilla-b2g30?
Attachment #8526403 - Flags: approval-mozilla-b2g30-
Reproduced the original issue using the following build: - http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1416229949/ Went through verification using the following build(s): - http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1420560231/ - http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1420582766/ - http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1420507584/ For each of the above builds, opened the testcase.html file from comment #0 and let it run for about 2 minutes. Ensured there weren't any crashes/issues.
Whiteboard: [adv-main35+] → [adv-main35+][b2g-adv-main2.2+]
You need to log in before you can comment on or make changes to this bug.