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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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)

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: bisection window

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Luke Wagner [:luke]]

I'm mostly on PTO; also shu rewrote this code recently, I believe, so he'd have a better idea.

Comment 7

[Shu-yu Guo [:shu]]

I am also on PTO until next week.

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Jan de Mooij [:jandem]]

(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.

Comment 10

[Jan de Mooij [:jandem]]

(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.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch

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`.

Comment 12

[Jan de Mooij [:jandem]]

(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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

(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?

Comment 14

[Andrew McCreight [:mccr8]]

Jandem, how bad is this security-wise?

Comment 15

[Jan de Mooij [:jandem]]

(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.

Comment 16

[Jan de Mooij [:jandem]]

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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8633474 [details] [diff] [review]
Patch

sec-approval+ for trunk. We should get this on branches too.

Comment 18

[Jan de Mooij [:jandem]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Jan de Mooij [:jandem]]

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.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8633474 [details] [diff] [review]
Patch

Let's get this on all branches in time for beta7.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c4b6599968

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/446ad5aa6877

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for esr38.

Comment 25

[Jan de Mooij [:jandem]]

(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.

Comment 26

[Jan de Mooij [:jandem]]

(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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9b60e57724db
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/9b60e57724db
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d03574a86b4c

Comment 28

[Christian Holler (:decoder)]

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