Closed Bug 1140196 Opened 5 years ago Closed 5 years ago

Crash [@ EnterNestedScope] or Assertion failure: getReservedSlot(LOCAL_OFFSET_SLOT).isUndefined(), at vm/ScopeObject.h

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox37 --- unaffected
firefox38 + verified
firefox39 + verified
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: gkw, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(5 keywords)

Crash Data

Attachments

(4 files)

Function("\
    for (var [x = (let(l)[])] = x in 0) {/x/}\
");

asserts js debug shell on m-c changeset 56492f7244a9 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: getReservedSlot(LOCAL_OFFSET_SLOT).isUndefined(), at vm/ScopeObject.h and crashes js opt shell at EnterNestedScope.

Opt configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/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 ~/fuzzing/js/compileShell.py -b "--disable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 56492f7244a9

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh ~/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 ~/fuzzing/js/compileShell.py -b "--enable-debug --enable-more-deterministic --enable-nspr-build -R ~/trees/mozilla-central" -r 56492f7244a9

In an opt build:

(lldb) dis -p
-> 0x100114f4a:  movl   0x30(%rdx,%rdx), %ecx
   0x100114f4e:  leaq   (%rdx,%rdx), %r12
   0x100114f52:  movzbl %cl, %edx
   0x100114f55:  shrl   $0x8, %ecx
(lldb) register read $rdx
     rdx = 0xfff9800000000000
(lldb) register read $ecx
     ecx = 0x02270a30
(lldb)

Not sure what's going on here, but weird memory addresses seem to be accessed here, so setting s-s and assuming sec-critical as a start.

The patch in bug 1127012 comment 9 does not seem to fix this issue.

autoBisect is running.
Flags: needinfo?
Attached file stack of opt crash
(lldb) bt 5
* thread #1: tid = 0x3992d7, 0x0000000100114f4a js-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(js::ExclusiveContext*, js::frontend::BytecodeEmitter*, js::frontend::StmtInfoBCE*, js::frontend::ObjectBox*, js::frontend::StmtType) [inlined] js::frontend::UpvarCookie::level(ts=0x00007fff5fbfcf10, newLevel=<unavailable>, newSlot=<unavailable>) const at ParseNode.h:47, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x0000000100114f4a js-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(js::ExclusiveContext*, js::frontend::BytecodeEmitter*, js::frontend::StmtInfoBCE*, js::frontend::ObjectBox*, js::frontend::StmtType) [inlined] js::frontend::UpvarCookie::level(ts=0x00007fff5fbfcf10, newLevel=<unavailable>, newSlot=<unavailable>) const at ParseNode.h:47
    frame #1: 0x0000000100114f4a js-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(js::ExclusiveContext*, js::frontend::BytecodeEmitter*, js::frontend::StmtInfoBCE*, js::frontend::ObjectBox*, js::frontend::StmtType) [inlined] ComputeAliasedSlots(root=0x0000000102d00258, dummy=<unavailable>) + 185 at BytecodeEmitter.cpp:843
    frame #2: 0x0000000100114e91 js-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(cx=0x0000000102d00240, bce=0x00007fff5fbfd870, stmt=0x00007fff5fbfc270, objbox=<unavailable>, stmtType=STMT_BLOCK) + 497 at BytecodeEmitter.cpp:956
    frame #3: 0x00000001001136e7 js-64-dm-nsprBuild-darwin-56492f7244a9`EnterBlockScope(cx=0x0000000102d00240, bce=0x00007fff5fbfd870, stmtInfo=<unavailable>, objbox=0x000000010300e650, initialValueOp=<unavailable>, alreadyPushed=<unavailable>) + 183 at BytecodeEmitter.cpp:2690
    frame #4: 0x00000001000fa7f2 js-64-dm-nsprBuild-darwin-56492f7244a9`EmitLet(cx=<unavailable>, bce=<unavailable>, pnLet=<unavailable>) + 162 at BytecodeEmitter.cpp:4838
Flags: needinfo?
(lldb) bt 5
* thread #1: tid = 0x39bd26, 0x00000001001e6869 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`js::StaticBlockObject::setLocalOffset(this=<unavailable>, offset=<unavailable>) + 409 at ScopeObject.h:611, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001001e6869 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`js::StaticBlockObject::setLocalOffset(this=<unavailable>, offset=<unavailable>) + 409 at ScopeObject.h:611
    frame #1: 0x00000001001c66d4 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(js::ExclusiveContext*, js::frontend::BytecodeEmitter*, js::frontend::StmtInfoBCE*, js::frontend::ObjectBox*, js::frontend::StmtType) [inlined] ComputeLocalOffset(root=0x0000000101e02098, bce=0x00007fff5fbfc630, dummy=<unavailable>) + 724 at BytecodeEmitter.cpp:894
    frame #2: 0x00000001001c6558 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`EnterNestedScope(cx=0x0000000101e02080, bce=0x00007fff5fbfc630, stmt=0x00007fff5fbfb7c0, objbox=<unavailable>, stmtType=STMT_BLOCK) + 344 at BytecodeEmitter.cpp:954
    frame #3: 0x00000001001c4787 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`EnterBlockScope(cx=0x0000000101e02080, bce=0x00007fff5fbfc630, stmtInfo=0x00007fff5fbfb7c0, objbox=0x0000000103814e50, initialValueOp=<unavailable>, alreadyPushed=<unavailable>) + 215 at BytecodeEmitter.cpp:2690
    frame #4: 0x000000010018db67 js-dbg-64-dm-nsprBuild-darwin-56492f7244a9`EmitLet(cx=0x0000000101e02080, bce=0x00007fff5fbfc630, pnLet=<unavailable>) + 311 at BytecodeEmitter.cpp:4838
(lldb)
Attachment #8573599 - Attachment description: stack → stack of assertion failure
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8832848bf234
user:        Arpad Borsos
date:        Tue Dec 09 10:33:51 2014 +0100
summary:     Bug 932080 - part1: support default values in destructuring of array and full objects; r=jorendorff

:Waldo was looking into possibly-related bugs, so setting needinfo? from him instead.
Blocks: 932080
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(jwalden+bmo)
Apparently I misunderstood/misread the callers of cloneParseTree -- this one weird case involving defaults admits arbitrary expressions and child nodes, without including a computed property name anywhere.  Kill it with fire, same reason.  (And kill all the callers of cloneLeftHandSide with fire as soon after as possible, but not in this bug.)

For background, particularly for shu if he ends up reviewing this faster: see bug 1127012 comment 9, bug 1127012 comment 8 (that the syntaxes there *are* the only ones where this cloning is invoked, it's just not all call stacks that enter cloneParseTree with an unconstrained node are necessarily within a computed property name as that bug thought), and bug 1127012 comment 12.

I have not verified that this change doesn't break any tests.  Seems best to get this patch posted, then work on finding what if anything was broken in parallel.  To some degree we're on the clock now -- I only learned about this bug about half an hour after landing bug 1127012's patch.  :-(  By the text of the patch it's not suggested that destructuring defaults might also be a fertile plain to investigate.  But someone who looked at callers closely to see what I missed, or who just threw enough fuzzing at the patterns in bug 1127012 comment 8, could eventually recognize this as an alternative path into the same broken code.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8573664 - Flags: review?(shu)
Attachment #8573664 - Flags: review?(jorendorff)
Duplicate of this bug: 1126555
Duplicate of this bug: 1126518
Sigh.  I misread Parser::forStatement (shocking, I know).  The cloning code is invoked for any for-loop beginning with |for (var ... in| or |for (var ... of| or |for (let ... in| or |for (let ... of|, regardless whether ... contains an assignment or not.  So technically, this isn't supposed to happen:

js> for (var { ["foo"]: x} in {}) ;
typein:1:11 SyntaxError: computed property names aren't supported in this destructuring declaration:
typein:1:11 for (var { ["foo"]: x} in {}) ;
typein:1:11 ...........^

And as for this bug, this patch breaks this existing test:

jit-test/tests/basic/destructuring-default.js:170:13 SyntaxError: destructuring defaults aren't supported in this destructuring declaration:
jit-test/tests/basic/destructuring-default.js:170:13 for (var [a, b = 10] in " ") {}
jit-test/tests/basic/destructuring-default.js:170:13 .............^

So technically, the patch in bug 1127012 and the patch here both break valid code.  I retreat to my fallback position: the cloning code is fundamentally unfixable; the syntax, while valid, is quite obscure; we haven't "supported" this syntax for all that long; and there is no trivial fix to this problem.  We should still make this change.  Not cloning would require making a bunch of changes to the bytecode emitter to support all this, changes that are far riskier than just disabling this syntax while it's fresh, changes that there's no chance in the world I'd ever recommend for backporting.
Duplicate of this bug: 1131267
Duplicate of this bug: 1133354
Duplicate of this bug: 1125658
Comment on attachment 8573664 [details] [diff] [review]
Kill more non-standard syntax, same approach as bug 1127012 for an alternative path into the same code that's buggy when subjected to arbitrary expressions

Review of attachment 8573664 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with comment 7.
Attachment #8573664 - Flags: review?(jorendorff) → review+
Comment on attachment 8573664 [details] [diff] [review]
Kill more non-standard syntax, same approach as bug 1127012 for an alternative path into the same code that's buggy when subjected to arbitrary expressions

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

As with bug 1127012, no idea.  The variety of dups of this bug, crashing in a variety of places, indicates just how poorly we understand this code.  Anything is possible.

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

No more or less so than in bug 1127012.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?

Bug 932080 added defaults syntax here.  It's not marked fixed, so I'm not sure when it landed, exactly, but bug 932080 comment 57 suggests maybe mid-January, which would mean this goes back to 38...if my math is right, which would need to be checked.  So not far, and not even in a release.

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

Easy to create.

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

Shouldn't break anything but what we *mean* to break.  Given this isn't in anything but aurora, shouldn't be too risky of breaking working code, either.
Attachment #8573664 - Flags: review?(shu) → sec-approval?
Comment on attachment 8573664 [details] [diff] [review]
Kill more non-standard syntax, same approach as bug 1127012 for an alternative path into the same code that's buggy when subjected to arbitrary expressions

sec-approval+ for trunk. We should get this nominated for Aurora too (or a new patch if needed).
Attachment #8573664 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/39488bb38d8d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Please nominate this for Aurora uplift when you get a chance.
Comment on attachment 8573664 [details] [diff] [review]
Kill more non-standard syntax, same approach as bug 1127012 for an alternative path into the same code that's buggy when subjected to arbitrary expressions

As noted in bug 1127012 comment 19, the Xdr.h changes will have to be handled a bit unusually by me, and they can't be autoland-backported.  Rest of the patch seems to apply trivially, tho.

I examined current mozilla-beta, and indeed the destructuring defaults code isn't there, so aurora is definitely it for backporting.

Approval Request Comment
[Feature/regressing bug #]: bug 932080, destructuring default expression values

[User impact if declined]: going off the rails in totally-un-understood code, triggering crashes in a variety of places, with very difficult-to-ascertain consequences quite possibly up to full-blown exploits

[Describe test coverage new/current, TreeHerder]: One test that verified destructuring expression defaults worked in for-in/of loop heads had to be disabled to avoid a test failure/syntax error, so there's that to show it works.

[Risks and why]: Very little, just adds a new syntax error, and that for either non-standard syntaxes, or for very-obscure valid syntax that we've only supported in current-Aurora at earliest.

[String/UUID change made/needed]: N/A
Flags: needinfo?(jwalden+bmo)
Attachment #8573664 - Flags: approval-mozilla-aurora?
Comment on attachment 8573664 [details] [diff] [review]
Kill more non-standard syntax, same approach as bug 1127012 for an alternative path into the same code that's buggy when subjected to arbitrary expressions

bug 1127012 backport has landed, approving for aurora uplift.
Attachment #8573664 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx38
Group: javascript-core-security
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.