424942 - int16 overflow with call object short ids

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Igor Bukanov]

Currently the code implementing call object in jsfun.c uses short id to store the index of an argument or variable. The id is accessed like in:

static JSBool
call_setProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    jsint slot;
...
    slot = JSVAL_TO_INT(id);

...
        if ((uintN)slot < JS_MAX(fp->argc, FUN_TO_SCRIPTED(fp->fun)->nargs))
            fp->argv[slot] = *vp;
...
}
...
JSBool
js_SetCallVariable(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
...
        JS_ASSERT((uintN) JSVAL_TO_INT(id) < fp->nvars);
        fp->vars[JSVAL_TO_INT(id)] = *vp;
...
}

That is, the id is interpreted as int. Yet the slot index in SpiderMonkey is bounded by 0xFFFF. Thus for any index beyond 0x7FFF in the above fragments JSVAL_TO_INT(id) will be a negative number. In the first fragment this leads to inaccessible locals. In the second fragment the result is a write outside allocated memory. 

Here is a test case demonstarting the problem. It constructs a function with 0XFFFF arguments and variables and read/write them from a closure:

~/m/ff/mozilla/js/src $ cat ~/s/x.js
function test(N)
{
    var probe;
    var source = [];

    source.push('function f(');
    for (var i = 0; i != N; ++i) {
        source.push((i == 0) ? 'a' : ',a');
        source.push(i);
    }
    source.push(') {');
    for (var i = 0; i != N; ++i) {
        source.push('var v');
        source.push(i);
        source.push('=a');
        source.push(i);
        source.push(';');
    }

    source.push('(function() { probe = ++v');
    source.push(N-1);
    source.push('+ ++a');
    source.push(N-1);
    source.push('; })();');
    source.push('return function() { return ++v');
    source.push(N-1);
    source.push('+ ++a');
    source.push(N-1);
    source.push('; } }');

    eval(source.join(''));
    var a = Array(N);
    a[N - 1] = 1;
    var x = f.apply(this, a)();
    if (probe !== 4)
        throw "Unexpected result:"+uneval(probe);
    if (x !== 6)
        throw "Unexpected result:"+uneval(x);
}

test(1<<15);
test((1<<16) - 1);

print('Done');

~/m/ff/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/x.js
Assertion failure: (uintN) JSVAL_TO_INT(id) < fp->nvars, at jsfun.c:729
trace/breakpoint trap

Comment 1

[Igor Bukanov]

On the trunk I will address the problem withing a patch for bug 411575. 

On 1.8.* branches the problem is rather mild as the code protects reads/writes with explicit checks. Thus on the branches the issue is inability to access from closures the arguments and locals with index beyond 32K. This can happen only with machine-generated scripts.

Comment 2

[Igor Bukanov]

Attachment: v1

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Created an attachment (id=311762) [details]
> v1
> 

The patch uses the relevant parts of the patch from bug 411575 to have separated getters and setters for all call object properties. This way the getter/setter can use full 64K range available in the short id. 

Comment 4

[Igor Bukanov]

Attachment: v2

The new version uses sfun, not fun, as the name for JSScriptedFunction local in call_resolve and casts the id to int16, not int, to make it explicit that short id is int16, not int.

Comment 5

[Brandon Sterne (:bsterne)]

this should block 1.9.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 311764 [details] [diff] [review]
v2

>+    if (sprop->getter == js_GetCallArg) {
>+        pd->slot = sprop->shortid;
>+        pd->flags |= JSPD_ARGUMENT;
>+    } else if (sprop->getter == js_GetCallVariable) {

Suggest s/Variable/Var/g appropriately to match.

Looks great otherwise -- thanks!

/be

Comment 7

[Brendan Eich [:brendan]]

Sorry for the mix-up, nominate as needed.

/be

Comment 8

[Mike Schroepfer]

Can we add test coverage for this before we land?

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)
> Can we add test coverage for this before we land?
> 

I can add a test based on Igor's in comment 0 however I normally don't add tests until they are fixed, but that's not a big deal. However, for security sensitive tests I normally create the test and attach it to the bug rather than check it in. This means that the test won't be run in the public JavaScript automated tests, but relies on me (and others who have access to the bug ) running it on local machines. Guidance please.

Comment 10

[Igor Bukanov]

Attachment: v2b

The new version addresses the nits from the comment 6.

Comment 11

[Mike Schroepfer]

Comment on attachment 312346 [details] [diff] [review]
v2b

k - bc can we get this covered after landing.

Comment 12

[Igor Bukanov]

Attachment: v3

Backing out of the patches for bug 424376 and bug 423874 required some trivial merging for the patch.

Comment 13

[Igor Bukanov]

I checked in the patch from the comment 12 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206805680&maxdate=1206805737&who=igor%25mir2.org

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-424942.js

I got bp-c37bdee6-fddd-11dc-9d71-001a4bd43 in today's minefield with this testcase, but it's a 404 so far.

debug minefield, linux 64:

#7  0x00002aaaaae2dae1 in JS_Assert (
    s=0x2aaaaae56fd0 "(uintN) JSVAL_TO_INT(id) < fp->nvars", 
    file=0x2aaaaae56d88 "/work/mozilla/builds/1.9.0/mozilla/js/src/jsfun.c", 
    ln=729) at /work/mozilla/builds/1.9.0/mozilla/js/src/jsutil.c:63
#8  0x00002aaaaad99606 in js_GetCallVariable (cx=0x1240b800, obj=0x126f01c0, 
    id=-3, vp=0x179fb120)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/jsfun.c:729
#9  0x00002aaaaadd8b95 in js_NativeGet (cx=0x1240b800, obj=0x126f01c0, 
    pobj=0x126f01c0, sprop=0x13139d38, vp=0x179fb120)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/jsobj.c:3537
#10 0x00002aaaaadd9a2b in js_GetPropertyHelper (cx=0x1240b800, obj=0x126f01c0, 
    id=395803252, vp=0x179fb120, entryp=0x0)
    at /work/mozilla/builds/1.9.0/mozilla/js/src/jsobj.c:3687
#11 0x00002aaaaadd9ae7 in js_GetProperty (cx=0x1240b800, obj=0x126f01c0, 
    id=395803252, vp=0x179fb120)

Comment 15

[Bob Clary [:bc] (inactive)]

v 1.9.0; 
Unexpected result:NaN on opt 1.8.1 
Assertion failure: JSVAL_IS_INT(id) on debug 1.8.1

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-424942.js

updated per Igor's instructions in bug 451721

Comment 17

[Graydon Hoare :graydon]

Hey, dropping in to add an amendment-recommendation on the test in this bug, provoked by bug 482554.

What's causing the latter is a change due to rev http://hg.mozilla.org/tracemonkey/rev/b8f929cd479d that adds an imacro. That imacro definition causes the JS emitter to reserve 3 'extra' stack slots in the frame, in order for the imacro to execute a JSOP_CALL, as far as I can tell. This means that your calculation in the testcase  js1_5/extensions/regress-424942.js above is off by 3. If you replace your threshold value "(1<<16) - 3" with "(1<<16) - 6", it works once more on the current tracemonkey tip.

IOW I think the bug is just that the testcase needs adjustment. I'm closing the 'regression' bug as invalid due to proposing the 'fix' happen over here instead; hope this is correct procedure.

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-424942.js

Comment 19

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.