Closed Bug 1182711 Opened 9 years ago Closed 9 years ago

Crash [@ js::ScopeIter::operator++] or Assertion failure: ssi_.type() == StaticScopeIter<CanGC>::Function, at vm/ScopeObject.cpp

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + verified
firefox41 + verified
firefox42 + verified
firefox43 --- verified
firefox-esr31 --- wontfix
firefox-esr38 40+ fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: gkw, Assigned: jandem)

Details

(5 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+])

Crash Data

Attachments

(4 files)

for (var a of [0]) {}
(function() {
    for (let b of [0]) {
        for (var c of null) {
            return;
            function f() {}
        }
    }
})()

asserts js debug shell on m-c changeset 7ec3e4b2a45f with --fuzzing-safe --no-threads --ion-eager at Assertion failure: ssi_.type() == StaticScopeIter<CanGC>::Function, at vm/ScopeObject.cpp and crashes opt shells at js::ScopeIter::operator++.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build" -r 7ec3e4b2a45f

Opt configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --disable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build" -r 7ec3e4b2a45f

A bisection window is coming up. :bz, :jorendorff and :shu landed stuff in the window so setting needinfo? from Boris as a start.

Setting s-s to be safe, this may merely be a null deref. Please feel free to open it up if it is not s-s.
Flags: needinfo?(bzbarsky)
Attached file debug stack
(lldb) bt 5
* thread #1: tid = 0xca491, 0x0000000100302253 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::settle(this=<unavailable>) + 1987 at ScopeObject.cpp:1052, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100302253 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::settle(this=<unavailable>) + 1987 at ScopeObject.cpp:1052
    frame #1: 0x0000000100302643 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::ScopeIter(this=0x00007fff5fbfcb90, cx=<unavailable>, frame=<unavailable>, pc=<unavailable>, _notifier=0x00007fff5fbfd618) + 387 at ScopeObject.cpp:1021
    frame #2: 0x0000000100544a27 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::HandleException(js::jit::ResumeFromException*) + 783 at JitFrames.cpp:740
    frame #3: 0x0000000100544718 js-dbg-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::HandleException(rfe=0x00007fff5fbfd6b8) + 2168 at JitFrames.cpp:894
    frame #4: 0x0000000101fb163d
(lldb)
Attached file opt stack
(lldb) bt 5
* thread #1: tid = 0xca592, 0x00000001001cd1e6 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::operator++() [inlined] JSObject::getClass(this=0x0000000000000000) const at jsobj.h:121, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001001cd1e6 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::operator++() [inlined] JSObject::getClass(this=0x0000000000000000) const at jsobj.h:121
    frame #1: 0x00000001001cd1e6 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::operator++() [inlined] bool JSObject::is<js::DeclEnvObject>(this=0x0000000000000000) const at jsobj.h:541
    frame #2: 0x00000001001cd1e6 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::ScopeIter::operator++(this=0x00007fff5fbfd210) + 38 at ScopeObject.cpp:1091
    frame #3: 0x0000000100177ba3 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::UnwindAllScopesInFrame(cx=<unavailable>, si=<unavailable>) + 35 at Interpreter.cpp:1209
    frame #4: 0x0000000100450c99 js-64-dm-nsprBuild-darwin-7ec3e4b2a45f`js::jit::DebugEpilogue(cx=0x0000000101aa90f0, frame=0x00007fff5fbfde58, pc=0x0000000101c903d3, ok=<unavailable>) + 137 at VMFunctions.cpp:696
(lldb)
changeset:   https://hg.mozilla.org/mozilla-central/rev/72accc37764a
user:        Boris Zbarsky
date:        Fri Mar 20 21:34:18 2015 -0400
summary:     Bug 1145491 part 6.  Fix script cloning to propagate the polluted-global-scope state to the lambda templates in the script.  r=luke

This particular changeset also mentions scope.
OK, so when the assert fired we have ssi_.type() == js::StaticScopeIter<1>::Block.

I really don't know very much about this scope chain stuff, unfortunately.  :(  Luke, do you have time to look into this?
Flags: needinfo?(bzbarsky) → needinfo?(luke)
I'm mostly on PTO; also shu rewrote this code recently, I believe, so he'd have a better idea.
Flags: needinfo?(luke) → needinfo?(shu)
Crash Signature: [@ js::ScopeIter::operator++]
I am also on PTO until next week.
At first glance it looks like we're in HandleExceptionBaseline, have a heavyweight function, but don't have a call object so we assert. I'll see if I can find out why we don't have a call object there.
(In reply to Jan de Mooij [:jandem] from comment #8)
> At first glance it looks like we're in HandleExceptionBaseline, have a
> heavyweight function, but don't have a call object so we assert. I'll see if
> I can find out why we don't have a call object there.

The problem is that IonBuilder calls `analysis().usesScopeChain()` and it returns false for this script.

After we bailout, FinishBailoutToBaseline will usually create a call object for the BaselineFrame if we need one (the EnsureHasScopeObjects call), but we only do that for the top frame and here it's a different frame. The fix is probably to call EnsureHasScopeObjects for all frames we bailed.

I also wonder if the usesScopeChain() optimization is buying us much these days; I'll do some measurements.
(In reply to Jan de Mooij [:jandem] from comment #9)
> I also wonder if the usesScopeChain() optimization is buying us much these
> days; I'll do some measurements.

Hm, it looks like on Octane/Sunspider/Kraken, there's no case where usesScopeChain() returns false for a heavyweight function... That suggests we can remove this mechanism and have Ion simply use fun->isHeavyweight() instead. That'd be a pretty nice simplification.
Flags: needinfo?(shu) → needinfo?(jdemooij)
Attached patch PatchSplinter Review
To remove usesScopechain() completely, we'd probably need recover instructions for MCallee and MFunctionEnvironment. This should be doable but is better done as a follow-up.

This patch ensures usesScopechain() is always `true` for heavyweight functions. As I mentioned in comment 10, this should not affect any of the main benchmarks but it does fix the weird case we have here: a heavyweight function that didn't get a CallObject because usesScopechain is `false`.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8633474 - Flags: review?(bhackett1024)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #4)
> changeset:   https://hg.mozilla.org/mozilla-central/rev/72accc37764a
> user:        Boris Zbarsky
> date:        Fri Mar 20 21:34:18 2015 -0400
> summary:     Bug 1145491 part 6.  Fix script cloning to propagate the
> polluted-global-scope state to the lambda templates in the script.  r=luke
> 
> This particular changeset also mentions scope.

I can reproduce this assertion failure on ESR38, FWIW.
Attachment #8633474 - Flags: review?(bhackett1024) → review+
(In reply to Jan de Mooij [:jandem] from comment #12)
> I can reproduce this assertion failure on ESR38, FWIW.

Any idea how far back this actually goes then?
Flags: needinfo?(jdemooij)
Jandem, how bad is this security-wise?
(In reply to Andrew McCreight [:mccr8] from comment #14)
> Jandem, how bad is this security-wise?

The dynamic scope chain is not what we expect it to be, it's hard to say how this affects the rest of the engine. I'll go with sec-high just to be safe.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #13)
> Any idea how far back this actually goes then?

It's hard to say but the patch is pretty simple so I think we should just backport it to all branches.
Flags: needinfo?(jdemooij)
Keywords: sec-high
Comment on attachment 8633474 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily. Maybe somebody who knows what they're doing but it's pretty hard.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

> Which older supported branches are affected by this flaw?

Assuming all of them.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Should be easy.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely, a green Try run should be enough.
Attachment #8633474 - Flags: sec-approval?
Comment on attachment 8633474 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get this on branches too.
Attachment #8633474 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/f083fd25e8fa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8633474 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Unknown, older Ion bug.
[User impact if declined]: Crashes, correctness bugs.
[Describe test coverage new/current, TreeHerder]: On mozilla-central.
[Risks and why]: Very small patch. It makes a certain optimization more conservative and predictable, should be safe.
[String/UUID change made/needed]: None.
Attachment #8633474 - Flags: approval-mozilla-esr38?
Attachment #8633474 - Flags: approval-mozilla-beta?
Attachment #8633474 - Flags: approval-mozilla-aurora?
Comment on attachment 8633474 [details] [diff] [review]
Patch

Let's get this on all branches in time for beta7.
Attachment #8633474 - Flags: approval-mozilla-esr38?
Attachment #8633474 - Flags: approval-mozilla-esr38+
Attachment #8633474 - Flags: approval-mozilla-beta?
Attachment #8633474 - Flags: approval-mozilla-beta+
Attachment #8633474 - Flags: approval-mozilla-aurora?
Attachment #8633474 - Flags: approval-mozilla-aurora+
Needs rebasing for esr38.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> Needs rebasing for esr38.

Working on it... The rebase itself is trivial but it doesn't fix this assert on esr38 (glad I checked), maybe there's something else we have to backport.
(In reply to Jan de Mooij [:jandem] from comment #25)
> Working on it... The rebase itself is trivial but it doesn't fix this assert
> on esr38 (glad I checked), maybe there's something else we have to backport.

We were eliminating the scope chain phi. We need to backport the fix in bug 1142331 to prevent that for heavyweight functions. Fortunately, that patch landed in 39 so I just rolled the trivial fix into this one:

https://hg.mozilla.org/releases/mozilla-esr38/rev/f39cb47bf0d9
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+]
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx40
JSBugMon: This bug has been automatically verified fixed on Fx41
JSBugMon: This bug has been automatically verified fixed on Fx42
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.