350417 - Crash decompiling for "is not a function" message, involving an array comprehension [@ js_GetSrcNoteOffset]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

javascript:y = {}; (y.a = [2 for each (p in [])])();

crashes, at least in a debug build.

#0  0x0105b820 in js_GetSrcNoteOffset (sn=0x0, which=0) at js/src/jsemit.c:6160
#1  0x010c0198 in Decompile (ss=0xbfff5a1c, pc=0x259fabd7 "\a", nb=43) at js/src/jsopcode.c:1850
#2  0x010c55ec in js_DecompileCode (jp=0x259fabf0, script=0x259fa5a0, pc=0x259fabc0 ";", len=43) at js/src/jsopcode.c:3110
#3  0x010c67c0 in js_DecompileValueGenerator (cx=0x22d1cb30, spindex=-9, v=48370528, fallback=0x2844810) at js/src/jsopcode.c:3462
#4  0x010699f0 in js_ReportIsNotFunction (cx=0x22d1cb30, vp=0x2e4e62c, flags=0) at js/src/jsfun.c:2251
#5  0x010b5db0 in ReportIsNotFunction (cx=0x22d1cb30, vp=0x2e4e62c, flags=0) at js/src/jsobj.c:4160
#6  0x010b6098 in js_Call (cx=0x22d1cb30, obj=0x2dc23f0, argc=0, argv=0x2e4e634, rval=0xbfff5d38) at js/src/jsobj.c:4233
#7  0x01073520 in js_Invoke (cx=0x22d1cb30, argc=0, flags=0) at js/src/jsinterp.c:1350
#8  0x01088eb8 in js_Interpret (cx=0x22d1cb30, pc=0x259fa60a ":", result=0xbfff6c60) at js/src/jsinterp.c:4047
#9  0x0107439c in js_Execute (cx=0x22d1cb30, chain=0x2dc23f0, script=0x259fa5a0, down=0x0, flags=0, result=0xbfff6dc0) at js/src/jsinterp.c:1599
#10 0x010232f0 in JS_EvaluateUCScriptForPrincipals (cx=0x22d1cb30, obj=0x2dc23f0, principals=0x259da724, chars=0xbfff6ec4, length=41, filename=0xbfff704c "javascript:y = {}; (y.a = [2 for each (p in [])])();", lineno=1, rval=0xbfff6dc0) at js/src/jsapi.c:4364

Comment 1

[Brendan Eich [:brendan]]

Minimal derived testcase showing decompilation bug, but not null pointer crash.

/be

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> Minimal derived testcase showing decompilation bug, but not null pointer crash.

Oops, forgot to paste it:

function g(){([2 for (p in [])]);}

Remove the parentheses around the array comprehension and all is well.  This testcase therefore demonstrates bug 350531.

Still, the null pointer crash when the unreduced testcase in comment 0 is evaluated as a top-level script, instead of as a function body, is a separate problem that should be tracked by this bug.

/be

Comment 3

[Jesse Ruderman]

> function g(){([2 for (p in [])]);}

What bad thing happens with that testcase?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> > function g(){([2 for (p in [])]);}
> 
> What bad thing happens with that testcase?

Nothing, I was running into my own bad patch to jsopcode.c (see bug 350531).

/be

Comment 5

[Jesse Ruderman]

js> for(a in (let (b=1) 2).c(3)) { };

also crashes deep in js_ReportIsNotFunction.  If this should be a separate bug, please split it for me.

#0  0x900030e8 in strlen ()
#1  0x00028cec in cvt_s (ss=0xbfffd51c, s=0xdc5c250a <Address 0xdc5c250a out of bounds>, width=0, prec=-1, flags=0) at jsprf.c:390
#2  0x0002a570 in dosprintf (ss=0xbfffd51c, fmt=0x120318 " = %s", ap=0xbfffd5d8 "\001\201J71") at jsprf.c:1008
#3  0x0002aba0 in JS_vsmprintf (fmt=0x120314 "%s%s = %s", ap=0xbfffd5d0 "") at jsprf.c:1156
#4  0x0002f17c in Sprint (sp=0xbfffd8fc, format=0x120314 "%s%s = %s") at jsopcode.c:421
#5  0x00035638 in Decompile (ss=0xbfffd8fc, pc=0x503315 "?", nb=18) at jsopcode.c:2039
#6  0x0003a120 in js_DecompileCode (jp=0x503650, script=0x5032e0, pc=0x503311 "?", len=18) at jsopcode.c:3149
#7  0x0003b30c in js_DecompileValueGenerator (cx=0x500180, spindex=-8, v=-2147483647, fallback=0x1804c08) at jsopcode.c:3504
#8  0x00052b60 in js_ReportIsNotFunction (cx=0x500180, vp=0x181622c, flags=0) at jsfun.c:2251

Comment 6

[Robert Sayre]

(In reply to comment #5)
> js> for(a in (let (b=1) 2).c(3)) { };

We're getting a bad pointer for lval here, i=2, rval="1":

>              case JSOP_SETLOCAL:
>                i = GET_UINT16(pc);
>                lval = OFF2STR(&ss->sprinter, ss->offsets[i]);
>                rval = POP_STR();
>                goto do_setlval;

Then, at do_setlval, we get a bad pointer for sn as well.

>              do_setlval:
>                sn = js_GetSrcNote(jp->script, pc - 1);
>                if (sn && SN_TYPE(sn) == SRC_ASSIGNOP) {
>                    todo = Sprint(&ss->sprinter, "%s %s= %s",
>                                  lval, js_CodeSpec[lastop].token, rval);
>                } else {
>                    sn = js_GetSrcNote(jp->script, pc);

Then the trouble happens in the call to Sprint below.

>                    todo = Sprint(&ss->sprinter, "%s%s = %s",
>                                  VarPrefix(sn), lval, rval);
>                }

Comment 7

[Robert Sayre]

(In reply to comment #6)
> (In reply to comment #5)
> > js> for(a in (let (b=1) 2).c(3)) { };
> We're getting a bad pointer for lval here...

after updating to today's src, I get a syntax error:

js> for(a in (let (b=1) 2).c(3)) { };
typein:3: SyntaxError: missing ) in parenthetical:
typein:3: for(a in (let (b=1) 2).c(3)) { };
typein:3: ....................^

Comment 8

[Robert Sayre]

(In reply to comment #0)
> javascript:y = {}; (y.a = [2 for each (p in [])])();

This still crashes for me when JSOP_FORLOCAL rolls around. We have "p" but calls to js_GetSrcNote are returning null.

Comment 9

[Brendan Eich [:brendan]]

Problem is js_DecompileValueGenerator mallocing memory to hold a slice of the script's bytecode, where some of the bytecode has source notes associated by offset from start of the script, not from start of the slice.  Worse, any offset computation of the form pc - script->code or pc - script->main (where script is ss->printer->script) will produce wild offsets.

I'd like to avoid the ancient (11 years? at least 10) js_DVG bytecode slicing.  More thought required.

/be

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > js> for(a in (let (b=1) 2).c(3)) { };
> > We're getting a bad pointer for lval here...
> 
> after updating to today's src, I get a syntax error:
> 
> js> for(a in (let (b=1) 2).c(3)) { };
> typein:3: SyntaxError: missing ) in parenthetical:
> typein:3: for(a in (let (b=1) 2).c(3)) { };
> typein:3: ....................^
> 

This is due to the conditioning of new keywords let and yield on JS version 1.7.  Use version(170) in the shell, <script type="application/javascript;version=1.7"> in HTML or XUL, to enable let.  See bug 351515.

/be

Comment 11

[Brendan Eich [:brendan]]

I think bug 351795 is essentially a dup of this one, and the problem again is js_DecompileValueGenerator slices bytecode and expects js_GetSrcNote to work.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: Fix js_DecompileValueGenerator to not rewrite bytecode

This means source notes work as usual.

The patch is not as big as it looks, consisting mainly of moving the rewriting code out of js_DecompileValueGenerator into the top of Decompile's main loop, where only op needs to be rewritten, nothing stored in shared-immutable script bytecode storage.

Instead of rewriting in memory, js_DecompileValueGenerator uses a JSPrinter flag to ask Decompile to rewrite op on the last iteration (it was always only the last bytecode that needed rewriting).

Note that destructuring decompilation is still crashy.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: updated and combined with bug 352392's patch

This avoids preempting two opcodes (not that we will run up against the 256 limit ever) for JSOP_GETPROP2 and JSOP_GETELEM2.

/be

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 238324 [details] [diff] [review]
updated and combined with bug 352392's patch

Fixed on trunk.

/be

Comment 15

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 16

[Mike Schroepfer]

Comment on attachment 238324 [details] [diff] [review]
updated and combined with bug 352392's patch

a=schrep

Comment 17

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 18

[Jesse Ruderman]

Comment 5 --> bug 352616.

Comment 19

[Jesse Ruderman]

And filed bug 352609 and bug 352613 on some other things that cause crashes deep within js_ReportIsNotFunction.

Comment 20

[Bob Clary [:bc] (inactive)]

Checking in regress-350417.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-350417.js,v  <--  regress-350417.js
initial revision: 1.1
done

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux