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

VERIFIED FIXED in Firefox 40, Firefox OS v2.1S

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla42
x86_64
Mac OS X
assertion, crash, regression, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox39 wontfix, firefox40+ verified, firefox41+ verified, firefox42+ verified, firefox43 verified, firefox-esr31 wontfix, firefox-esr3840+ 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)

Details

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

Attachments

(4 attachments)

(Reporter)

Description

3 years ago
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)
(Reporter)

Comment 1

3 years ago
Created attachment 8632366 [details]
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)
(Reporter)

Comment 2

3 years ago
Created attachment 8632367 [details]
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)
(Reporter)

Comment 3

3 years ago
Created attachment 8632368 [details]
bisection window
(Reporter)

Comment 4

3 years ago
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)

Comment 6

3 years ago
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)
(Reporter)

Updated

3 years ago
Crash Signature: [@ js::ScopeIter::operator++]

Comment 7

3 years ago
I am also on PTO until next week.
(Assignee)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
(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)
(Assignee)

Comment 11

3 years ago
Created attachment 8633474 [details] [diff] [review]
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`.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8633474 - Flags: review?(bhackett1024)
(Assignee)

Comment 12

3 years ago
(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+
(Reporter)

Updated

3 years ago
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
status-firefox-esr38: --- → affected
(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?
(Assignee)

Comment 15

3 years ago
(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
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox39: affected → wontfix
status-firefox-esr31: --- → wontfix
(Assignee)

Comment 16

3 years ago
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?
tracking-firefox40: --- → +
tracking-firefox41: --- → +
tracking-firefox42: --- → +
tracking-firefox-esr38: --- → 40+
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
Last Resolved: 3 years ago
status-b2g-master: affected → fixed
status-firefox42: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 20

3 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c4b6599968
status-b2g-v2.0: affected → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1S: --- → affected
status-b2g-v2.2r: --- → affected
status-firefox41: affected → fixed
Needs rebasing for esr38.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
(Assignee)

Comment 25

3 years ago
(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.
(Assignee)

Comment 26

3 years ago
(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
status-firefox-esr38: affected → fixed
Flags: needinfo?(jdemooij)
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
status-b2g-v2.1: affected → wontfix
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
status-b2g-v2.2r: affected → fixed
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+]
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
status-firefox41: fixed → verified
status-firefox42: fixed → verified
status-firefox43: --- → verified
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

Updated

2 years ago
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.