1186962 - Assertion failure: opn->isArity(PN_NAME), at frontend/ParseNode.cpp:852 or Crash [@ js::frontend::BytecodeEmitter::emitTree] with Arrow function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2ddec2dedced (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2):

for (var { __proto__: {  } = (  ) => current } in {}) {}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x0000000000555551 in js::frontend::BytecodeEmitter::emitTree (this=this@entry=0x7fffffffc530, pn=pn@entry=0x0) at js/src/frontend/BytecodeEmitter.cpp:7529
#0  0x0000000000555551 in js::frontend::BytecodeEmitter::emitTree (this=this@entry=0x7fffffffc530, pn=pn@entry=0x0) at js/src/frontend/BytecodeEmitter.cpp:7529
#1  0x0000000000558c5d in js::frontend::BytecodeEmitter::emitDefault (this=0x7fffffffc530, defaultExpr=0x0) at js/src/frontend/BytecodeEmitter.cpp:3820
#2  0x000000000055b417 in js::frontend::BytecodeEmitter::emitDestructuringOpsObjectHelper (this=this@entry=0x7fffffffc530, pattern=pattern@entry=0x7ffff69913a0, emitOption=emitOption@entry=js::frontend::InitializeVars) at js/src/frontend/BytecodeEmitter.cpp:4022
#3  0x000000000055adbb in js::frontend::BytecodeEmitter::emitDestructuringOpsHelper (this=this@entry=0x7fffffffc530, pattern=pattern@entry=0x7ffff69913a0, emitOption=emitOption@entry=js::frontend::InitializeVars) at js/src/frontend/BytecodeEmitter.cpp:4107
#4  0x000000000055a028 in emitDestructuringOps (isLet=false, pattern=0x7ffff69913a0, this=0x7fffffffc530) at js/src/frontend/BytecodeEmitter.cpp:4118
#5  js::frontend::BytecodeEmitter::emitAssignment (this=this@entry=0x7fffffffc530, lhs=0x7ffff69913a0, op=op@entry=JSOP_NOP, rhs=rhs@entry=0x0) at js/src/frontend/BytecodeEmitter.cpp:4580
#6  0x000000000055bdde in js::frontend::BytecodeEmitter::emitForIn (this=0x7fffffffc530, pn=<optimized out>, top=<optimized out>) at js/src/frontend/BytecodeEmitter.cpp:5492
#7  0x000000000055c4a5 in js::frontend::BytecodeEmitter::emitFor (this=this@entry=0x7fffffffc530, pn=pn@entry=0x7ffff69914b8, top=top@entry=0) at js/src/frontend/BytecodeEmitter.cpp:5708
#8  0x0000000000555898 in js::frontend::BytecodeEmitter::emitTree (this=this@entry=0x7fffffffc530, pn=0x7ffff69914b8) at js/src/frontend/BytecodeEmitter.cpp:7646
#9  0x0000000000556e45 in js::frontend::CompileScript (cx=cx@entry=0x7ffff6981040, alloc=<optimized out>, scopeChain=..., enclosingStaticScope=..., enclosingStaticScope@entry=..., evalCaller=..., evalCaller@entry=..., options=..., srcBuf=..., source_=source_@entry=0x0, staticLevel=staticLevel@entry=0, extraSct=extraSct@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:409
#10 0x000000000089ca36 in Compile (cx=cx@entry=0x7ffff6981040, options=..., scopeOption=scopeOption@entry=HasSyntacticScope, srcBuf=..., script=script@entry=...) at js/src/jsapi.cpp:3931
#11 0x000000000089cb3f in Compile (script=..., length=<optimized out>, chars=0x7ffff6902e80 u"for (var { __proto__: {  } = (  ) => current } in {}) {}\n", scopeOption=HasSyntacticScope, options=..., cx=0x7ffff6981040) at js/src/jsapi.cpp:3940
#12 Compile (cx=cx@entry=0x7ffff6981040, options=..., scopeOption=scopeOption@entry=HasSyntacticScope, bytes=<optimized out>, length=57, script=script@entry=...) at js/src/jsapi.cpp:3955
#13 0x00000000008c1fb1 in Compile (script=..., fp=0x7fffffffd880, scopeOption=HasSyntacticScope, options=..., cx=cx@entry=0x7ffff6981040) at js/src/jsapi.cpp:3966
#14 JS::Compile (cx=cx@entry=0x7ffff6981040, options=..., file=file@entry=0x7ffff6997800, script=script@entry=...) at js/src/jsapi.cpp:4006
#15 0x0000000000426063 in RunFile (compileOnly=false, file=0x7ffff6997800, filename=<optimized out>, cx=0x7ffff6981040) at js/src/shell/js.cpp:449
#16 Process (cx=cx@entry=0x7ffff6981040, filename=<optimized out>, forceTTY=forceTTY@entry=false) at js/src/shell/js.cpp:576
#17 0x000000000043657d in ProcessArgs (op=0x7fffffffdbc0, cx=0x7ffff6981040) at js/src/shell/js.cpp:5771
#18 Shell (envp=<optimized out>, op=0x7fffffffdbc0, cx=0x7ffff6981040) at js/src/shell/js.cpp:6040
#19 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6384
rax	0x7fffffffc5d8	140737488340440
rbx	0x7fffffffc530	140737488340272
rcx	0x51	81
rdx	0x7ffff693c0a0	140737330266272
rsi	0x0	0
rdi	0x7fffffffc530	140737488340272
rbp	0x0	0
rsp	0x7fffffffbf20	140737488338720
r8	0x7ffff691b6b0	140737330132656
r9	0x7ffff691b6b0	140737330132656
r10	0x7ffff6913400	140737330099200
r11	0x7ffff47ad680	140737295079040
r12	0x7ffff69913d8	140737330615256
r13	0x2	2
r14	0x7fffffffbf30	140737488338736
r15	0x1	1
rip	0x555551 <js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*)+113>
=> 0x555551 <js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*)+113>:	mov    0x4(%rbp),%esi
   0x555554 <js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*)+116>:	mov    0x10(%rax),%r13

Comment 1

[Christian Holler (:decoder)]

The signature of this one looks very similar to bug 1186952 but doesn't contain Classes. Still needinfo on Eric because they might be related.

Comment 2

[Christian Holler (:decoder)]

Here's another instance of this problem without arrow-function, but using for-of.

This is an automated crash issue comment:

Summary: Assertion failure: opn->isArity(PN_NAME), at js/src/frontend/ParseNode.cpp:852
Build version: mozilla-central revision d3228c82badd
Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug
Runtime options: --fuzzing-safe --thread-count=2 --ion-check-range-analysis

Testcase:

for (var { __proto__: set = () => v.value.BUGNUMBER = 2 } of objectWithProtoGenerator(null)) {}

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00000000005fb4dc in js::frontend::Parser<js::frontend::FullParseHandler>::cloneLeftHandSide (this=this@entry=0x7f5ff01c64f0, opn=0x7f5fed7c37c8) at js/src/frontend/ParseNode.cpp:852
#1  0x00000000005fb39c in js::frontend::Parser<js::frontend::FullParseHandler>::cloneLeftHandSide (this=this@entry=0x7f5ff01c64f0, opn=opn@entry=0x7f5fed7c3448) at js/src/frontend/ParseNode.cpp:808
#2  0x00000000004cebe5 in js::frontend::Parser<js::frontend::FullParseHandler>::forStatement (this=this@entry=0x7f5ff01c64f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:5025
#3  0x00000000004f118b in js::frontend::Parser<js::frontend::FullParseHandler>::statement (this=this@entry=0x7f5ff01c64f0, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:6209
#4  0x0000000000627ac8 in BytecodeCompiler::compileScript (this=this@entry=0x7f5ff01c5f20, scopeChain=..., scopeChain@entry=..., evalCaller=evalCaller@entry=..., staticLevel=staticLevel@entry=0) at js/src/frontend/BytecodeCompiler.cpp:605
#5  0x0000000000628103 in js::frontend::CompileScript (cx=<optimized out>, alloc=alloc@entry=0x7f5ff06dde10, scopeChain=scopeChain@entry=..., enclosingStaticScope=enclosingStaticScope@entry=..., evalCaller=evalCaller@entry=..., options=..., srcBuf=..., source_=source_@entry=0x0, staticLevel=staticLevel@entry=0, extraSct=extraSct@entry=0x0) at js/src/frontend/BytecodeCompiler.cpp:773
#6  0x0000000000681048 in js::HelperThread::handleParseWorkload (this=this@entry=0x7f5ff0633000) at js/src/vm/HelperThreads.cpp:1250
#7  0x0000000000681544 in js::HelperThread::threadLoop (this=0x7f5ff0633000) at js/src/vm/HelperThreads.cpp:1437
#8  0x0000000000701de1 in nspr::Thread::ThreadRoutine (arg=0x7f5ff06310e0) at js/src/vm/PosixNSPR.cpp:45
#9  0x00007f5ff1829182 in start_thread (arg=0x7f5ff01c7700) at pthread_create.c:312
#10 0x00007f5ff0918fbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111
rax	0x0	0
rbx	0x7f5fed7c3918	140049982961944
rcx	0x7f5ff090a3cd	140050034631629
rdx	0x0	0
rsi	0x7f5ff0bdf9d0	140050037602768
rdi	0x7f5ff0bde1c0	140050037596608
rbp	0x7f5ff01c5820	140050027010080
rsp	0x7f5ff01c57d0	140050027010000
r8	0x7f5ff01c7700	140050027017984
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7f5ff0bdbbe0	140050037586912
r11	0x0	0
r12	0x7f5ff01c64f0	140050027013360
r13	0x7f5fed7c37c8	140049982961608
r14	0x79	121
r15	0x2	2
rip	0x5fb4dc <js::frontend::Parser<js::frontend::FullParseHandler>::cloneLeftHandSide(js::frontend::ParseNode*)+1468>
=> 0x5fb4dc <js::frontend::Parser<js::frontend::FullParseHandler>::cloneLeftHandSide(js::frontend::ParseNode*)+1468>:	movl   $0x354,0x0
   0x5fb4e7 <js::frontend::Parser<js::frontend::FullParseHandler>::cloneLeftHandSide(js::frontend::ParseNode*)+1479>:	callq  0x498fe0 <abort()>

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

cc'ing parser people

Comment 4

[Jeff Walden [:Waldo]]

This looks like a rehash of bug 1127012 and/or bug 1140196, hiding pending deeper investigation.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch

Yeah, a straight-out replay of bug 1140196 as applied to properties named __proto__.  Why couldn't this have been reported about three weeks ago...oh wait.  :-(  I guess barring any notice of exploitation we're stuck sitting on this for landing/shipping for a month or so, now.

Comment 6

[Andrew McCreight [:mccr8]]

Marking sec-critical because the bug mentioned in comment 5 is sec-critical.

Comment 7

[Jason Orendorff [:jorendorff]]

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

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

Argh.

Comment 8

[Jeff Walden [:Waldo]]

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

I think we should sit on landing this for a month or so, unless (gulp) someone notices this lapse and does legwork converting it into something worse on us.  But I might as well get it on the radar now, at least.

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

As easily as for bug 1140196: "[N]o 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."  Note that the current patch doesn't augment the current tests with the special-case this bug disables.  That'll be done in a separate patch, eventually.

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

Everything affected by bug 1140196, so 38 onward.

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

Should be trivial/not risky.  Bug 1140196 disables the syntax in the general case.  This further disables a small sliver more where the property name is "__proto__".  Anyone who'd be hurt by this, would have been hurt already by bug 1140196, so this is just going to hurt deliberate attempts to crash/exploit us.

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

It shouldn't require special testing and won't regress anything but a very-particular targeted syntax.

Comment 9

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

Checkin on 9/1 or later. Sec-approval+.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/362fb8801d46

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/362fb8801d46

Please nominate this for Aurora/Beta/esr38 approval as soon as you get a chance.

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Add tests

Okay, this checks semantics for a wide range of cases if destructuring defaults are supported in a simple case, and otherwise it checks that all those cases are syntax errors if destructuring defaults are *not* supported in that simple case.  I probably should have written my test-fixes that way from the start.  :-\

Of course this patch isn't landable until some time after this bug's been fixt and in users' hands, but I felt guilty not offering this before making another approval request.  Particularly given something more comprehensive like this, checking/expecting a syntax error, might have uncovered the incompleteness of the previous fix earlier.

Comment 13

[Jeff Walden [:Waldo]]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
We're going off the rails in very-un-understood code, and it is entirely unclear what the consequences of the mistake here are.  Could be sec-critical, could be safe crashes, but I would never ever in a million years bet on that.

User impact if declined:
Crashes, possibly exploitable but none of us are smart enough to know.

Fix Landed on Version:
In trunk now, so 43.

Risk to taking this patch (and alternatives if risky): 
Narrowly disables some syntax, low risk.  No real alternatives, as fixing the relevant syntax is totally un-branch-shippable.

String or UUID changes made by this patch: N/A

Approval Request Comment
[Feature/regressing bug #]: bug 932080
[User impact if declined]: crashes, possibly exploitable but impossible to say without excessive investigation (and even then I don't think we could be confident in that investigation)
[Describe test coverage new/current, TreeHerder]: testcase posted in bug, fails without patch, passes with
[Risks and why]: very little, just turning a small bit of syntax into a syntax error -- should *prevent* squirrely code being hit, not make anything worse
[String/UUID change made/needed]: N/A

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/472861dd7df6

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6784f96467d8

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Stability fix; we should take this for ESR too.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/4ef28cbee8c2

Comment 18

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx41
JSBugMon: This bug has been automatically verified fixed on Fx42
JSBugMon: This bug has been automatically verified fixed on Fx43

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6cde155f4b
Add a test.  r=jorendorff

Comment 20

[Carsten Book [:Tomcat]]

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