418051 - "({x:[]}={x}" causes assertion failure in jsparse.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> eval("({x:[]}={x}")
Assertion failure: (pnkey)->pn_arity == PN_NULLARY && ((pnkey)->pn_type == TOK_NUMBER || (pnkey)->pn_type == TOK_STRING || (pnkey)->pn_type == TOK_NAME), at jsparse.c:1974

Opt doesn't blow up too badly.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Asking for dot-release scheduling; if nothing else, masks fuzzing of things like 433279, which would have been nice to catch before RC1 :/

Comment 2

[Jesse Ruderman]

It doesn't really mask that bug.  I can avoid this bug most of the time with a regexp and still have the fuzzer hit bug 433279.

Comment 3

[Samuel Sidler (old account; do not CC)]

Brian, who can own this?

Comment 4

[Brian Crowder]

The usual suspects would be Brendan, mrbkap, or perhaps Igor.  I'd look at it if I had time, but I'm largely occupied at the moment, and I've never spent much time in the parser.

Comment 5

[Samuel Sidler (old account; do not CC)]

Blake, can you take a look at this for 1.9.0.3?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

Brendan pointed me on the right track here. Without destructuring assignment, we catch this in the TOK_RC case in the emitter.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 334589 [details] [diff] [review]
Proposed fix

Yay! Let's get this in before the freeze tonight. Thanks,

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/af00b3f27c64

Comment 9

[Dave Camp (:dcamp)]

This patch is causing startup crashes:

1998	    if (right->pn_extra & PNX_SHORTHAND) {
(gdb) print right
$1 = (JSParseNode *) 0x0

Backed out:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e2f89ccef0e4

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better proposed fix

If I'd bothered reading any more of the rest of CheckDestructuring, I would have noticed that |right| is null checked before ever other use :-/

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 334701 [details] [diff] [review]
Better proposed fix

Oops, yeah -- sorry.

Nit: We traditionally parenthesize bitwise ops against almost all ops (not against assignment).

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Pushed to m-c as http://hg.mozilla.org/index.cgi/mozilla-central/rev/43a4c95537b8

Comment 13

[Bob Clary [:bc] (inactive)]

this caused js1_7/regress/regress-389605.js 

let ([] = function() { }) { try { throw 1; } catch(x) { } }

to throw a syntax error.

Intentional?

Comment 14

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/expressions/regress-418051.js,v  <--  regress-418051.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/477ca5117e8e

Comment 15

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

Attachment: backported patch to 1.9.0.x

The assert occurs pretty frequently when jsfunfuzz-ing 1.9.0.x. This patch backport fixes the assert.

Comment 16

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

Attachment: diff between trunk patch and the 1.9.0.x one.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 348414 [details] [diff] [review]
backported patch to 1.9.0.x

This is missing the followup fix from bug 453345.

Comment 18

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

Attachment: patch v2 for 1.9.0.x

Second pass at a backport.

Console output with this patch applied:

$ ./js
js> let ([] = function() { }) { try { throw 1; } catch(x) { } }
js> 

$ ./js
js> eval("({x:[]}={x}")
typein:1: SyntaxError: invalid object initializer:
typein:1: ({x:[]}={x}
typein:1: ........^
js>

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 348785 [details] [diff] [review]
patch v2 for 1.9.0.x

>+    if (right && right->pn_arity == PN_LIST && 
>+        (right->pn_extra & PNX_SHORTHAND)) {

Nit: SpiderMonkey style for the 1.9 branch wants:

if (right &&
    right->pn_arity == PN_LIST &&
    (right->pn_extra & PNX_SHORTHAND)) {
...
}

r=mrbkap with that.

Comment 20

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

Attachment: third pass

this should do it. bringing over r+.

Comment 21

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

Attachment: diff between both v2 and v3 1.9.0.x patches

diff between both v2 and v3 1.9.0.x patches just to be (very) safe.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 348893 [details] [diff] [review]
third pass

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Gary, are you going to land this patch?

Comment 24

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

(In reply to comment #23)
> Gary, are you going to land this patch?

Nope, I don't have checkin privs.

Comment 25

[Robert Sayre]

checked in for 1.9.0.6

Checking in js/src/jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.350; previous revision: 3.349
done

Comment 26

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1, 1.9.2