509354 - Crash [@ DecompileDestructuringLHS] with destructuring and "arguments"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

print(function([arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments,arguments]){})

Crash [@ DecompileDestructuringLHS] touching some wrong memory.

Related to bug 496985?

Comment 1

[Jesse Ruderman]

Attachment: crash stack trace

Comment 2

[Jesse Ruderman]

Attachment: valgrind's theory

Doesn't crash under valgrind, but does have an "Invalid read of size 1".

Comment 3

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

autoBisect shows this might be related to bug 496908 :

The first bad revision is:
changeset:   30693:6fc1700090b2
user:        Boris Zbarsky
date:        Mon Jul 27 16:14:12 2009 -0400
summary:     Bug 496908.  Make JSVAL_IS_* functions, not macros.  r=brendan,jwalden

Comment 4

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

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000001e101b
Crashed Thread:  0

Thread 0 Crashed:
0   libSystem.B.dylib             	0xffff07fc __memcpy + 92 (cpu_capabilities.h:246)
1   js-opt-tm-darwin              	0x0008734d DecompileDestructuringLHS(SprintStack*, unsigned char*, unsigned char*, int*) + 1213
2   js-opt-tm-darwin              	0x00087d60 DecompileDestructuring(SprintStack*, unsigned char*, unsigned char*) + 1072
3   js-opt-tm-darwin              	0x000883bd js_DecompileFunction + 381
4   js-opt-tm-darwin              	0x0000cdd3 JS_DecompileFunction + 67
5   js-opt-tm-darwin              	0x00043d39 fun_toStringHelper(JSContext*, unsigned int, unsigned int, long*) + 313
6   js-opt-tm-darwin              	0x0005d1c7 js_Invoke + 2407
7   js-opt-tm-darwin              	0x0005d6bb js_InternalInvoke + 139
8   js-opt-tm-darwin              	0x0006dd84 js_TryMethod + 212
9   js-opt-tm-darwin              	0x0006df76 js_DefaultValue + 374
10  js-opt-tm-darwin              	0x000c4e33 js_ValueToString + 131
11  js-opt-tm-darwin              	0x00002375 Print(JSContext*, unsigned int, long*) + 69
12  js-opt-tm-darwin              	0x00057016 js_Interpret + 42182
13  js-opt-tm-darwin              	0x0005c5f2 js_Execute + 370
14  js-opt-tm-darwin              	0x0000e1ac JS_ExecuteScript + 60
15  js-opt-tm-darwin              	0x0000432a Process(JSContext*, JSObject*, char*, int) + 1338
16  js-opt-tm-darwin              	0x000075bf main + 879
17  js-opt-tm-darwin              	0x00001b3b _start + 209
18  js-opt-tm-darwin              	0x00001a69 start + 41

Comment 5

[Jason Orendorff [:jorendorff]]

Two observations:

(function([developers,developers,developers,developers]){});
==> SyntaxError: duplicate argument is mixed with destructuring pattern
...so the correct behavior here is to throw;

script->nfixed is 0, so the decompiler isn't recognizing the `setlocalpop 0` instructions in the destructuring-assignment cliche as assigning to an argument that has a name.

Comment 6

[Jason Orendorff [:jorendorff]]

When BindDestructuringVar calls BindLocalVariable, it assumes that a successful return means there's a new local variable. But when atom is `arguments` this is not the case; BindLocalVariable just smiles and nods.

This is all kind of silly. Brendan, is it OK if we just reject functions with destructuring arguments named `argument` as a SyntaxError?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> When BindDestructuringVar calls BindLocalVariable, it assumes that a successful
> return means there's a new local variable. But when atom is `arguments` this is
> not the case; BindLocalVariable just smiles and nods.

Bug there -- notice without destructuring how you can bind a local var named arguments:

js> function f(x) { var arguments = x; return arguments }
js> dis(f)
flags: HEAVYWEIGHT
00000:  defvar "arguments"
main:
00003:  bindname "arguments"
00006:  getarg 0
00009:  setname "arguments"
00012:  pop
00013:  arguments
00014:  return
00015:  stop

Source notes:
  0:     2 [   2] xdelta  
  1:     9 [   7] decl     offset 0
js> f()
js> f(42)
42

/be

Comment 8

[Damon Sicore (:damons)]

Blocking 1.9.2+ P2.

Comment 9

[Damon Sicore (:damons)]

Assigning to blake.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I'm not going to get this before the freeze. Throwing back into the pool for someone else to catch.

Comment 11

[Jason Orendorff [:jorendorff]]

catching

Comment 12

[Damon Sicore (:damons)]

Progress, Jason?

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 413627 [details] [diff] [review]
v1

Please add a test that 'function([arguments])' results in a function that can actually use its parameter. r=mrbkap with that.

Comment 15

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/2cd3b54918b0

Comment 16

[Bob Clary [:bc] (inactive)]

jorendorff: js1_8_1/regress/regress-509354.js needs to be listed in the appropriate jstests.list manifest.

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2cd3b54918b0

Comment 18

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/99697799512c

Comment 19

[Robert Sayre]

(In reply to comment #16)
> jorendorff: js1_8_1/regress/regress-509354.js needs to be listed in the
> appropriate jstests.list manifest.

Did this ever happen?

Comment 20

[Bob Clary [:bc] (inactive)]

nope.

Comment 21

[Jim Blandy :jimb]

Hi, Jason.  It seems like js/src/tests/js1_8_1/regress/regress-509354.js still needs to be added to jstests.list.

Comment 22

[Bob Clary [:bc] (inactive)]

$ python jstests.py -c
Test files not contained in any manifest:
js1_8_1/regress/regress-509354.js

Comment 23

[Bob Clary [:bc] (inactive)]

jorendorff, still need to list this test in the manifest.

Comment 24

[Bob Clary [:bc] (inactive)]

fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1

Comment 25

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/2f7479402558

Comment 26

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_1/regress/regress-509354.js.