355556 - Crash [@ js_Interpret] with "arguments" as second param to eval

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jesse Ruderman]

js> (function () { eval("'foo'.b()", arguments) })()

In an opt js shell, this crashes [@ js_Interpret] with a null deref.

In a debug js shell, I get "Assertion failure: obj, at jsinterp.c:5722" instead.

0   js 	0x0000a070 JS_Assert + 84 (jsutil.c:60)
1   js 	0x000b0e84 js_Interpret + 111736 (jsinterp.c:5723)
2   js 	0x000944d4 js_Execute + 936 (jsinterp.c:1618)
3   js 	0x00042f04 obj_eval + 1824 (jsobj.c:1358)
4   js 	0x000938d0 js_Invoke + 3912 (jsinterp.c:1373)
5   js 	0x000a690c js_Interpret + 69376 (jsinterp.c:3923)
6   js 	0x000944d4 js_Execute + 936 (jsinterp.c:1618)
7   js 	0x00021518 JS_ExecuteScript + 64 (jsapi.c:4256)
8   js 	0x00003024 Process + 904 (js.c:265)
9   js 	0x00003bec ProcessArgs + 2304 (js.c:487)
10  js 	0x00009fdc main + 640 (js.c:3088)
11  js 	0x00002388 _start + 340 (crt.c:272)
12  js 	0x00002230 start + 60

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Igor Bukanov]

Comment on attachment 241419 [details] [diff] [review]
fix

>+
>+    /*
>+     * Give arguments an intrinsic scope chain link to fp->varobj.  Since the
>+     * arguments object lacks a prototype object because js_ArgumentsClass is
>+     * not initialized, js_NewObject won't assign a default parent to it.
>+     *
>+     * Therefore if arguments is used as the head of an eval scope chain (via
>+     * a direct call to eval(program, arguments)), any reference to a standard
>+     * object in the program will fail to resolve via js_GetClassPrototype not
>+     * finding a global object starting from arguments and following parent.
>+     */

An assert here about null parent here would be nice.

>+    argsobj->slots[JSSLOT_PARENT] = OBJECT_TO_JSVAL(fp->varobj);

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> An assert here about null parent here would be nice.
> 
> >+    argsobj->slots[JSSLOT_PARENT] = OBJECT_TO_JSVAL(fp->varobj);

In a lightweight function that uses arguments and not just arguments.length or arguments[i], fp->varobj will be null, and that's ok.  It is only via eval called by its name directly, but with two arguments, that arguments could replace the dynamic (caller's) scope chain.  And eval called by its name causes the compiler to flag the function JSFUN_HEAVYWEIGHT, which means it will get a non-null varobj.

So the assertion would have to be of the form

    JS_ASSERT(!fp->fun || !(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->varobj);

/be 

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
>     JS_ASSERT(!fp->fun || !(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->varobj);

No coffee yet, make that

    JS_ASSERT(fp->fun && (!(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->varobj));

/be

Comment 5

[Brendan Eich [:brendan]]

I'm wrong, I should know this by now: eval(s, o) replaces the scope chain with o, and so does indirect.eval(s, o) or evil = eval; evil(s, o):

evil = eval;
function f() { evil("'foo'.b()", arguments) }
f()

This will crash as in comment 0 with a null obj deref or equivalent obj-not-null assertion.

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 7

[Brendan Eich [:brendan]]

Attachment: fix, v2 with comments updated

Comment 8

[Brendan Eich [:brendan]]

Checked into trunk:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.167; previous revision: 3.166
done

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: fix, v2 with even better comments

Comment 10

[Bob Clary [:bc] (inactive)]

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

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20061007 no crash windows/linux

catch exception
Checking in regress-355556.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-355556.js,v  <--  regress-355556.js
new revision: 1.2; previous revision: 1.1
done

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 241484 [details] [diff] [review]
fix, v2 with even better comments

approved for 1.8 branch, a=dveditz for drivers

Comment 13

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsfun.c;
/cvsroot/mozilla/js/src/jsfun.c,v  <--  jsfun.c
new revision: 3.117.2.27; previous revision: 3.117.2.26
done

/be

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 20061122 1.8.1.1 windows/linux/mac*, 1.9 windows/linux