508178 - JIT can index into the wrong arguments object

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

function test() {
    var testargs = arguments;
    var f = function (name, which) {
        var args = [testargs, arguments];
        return args[which][0];
    };
    var arr = [0, 0, 0, 0, 1];
    for (var i = 0; i < arr.length; i++)
        arr[i] = f("f", arr[i]);
    assertEq("" + arr, ",,,,f");
}
test();

In general the JIT seems to assume the record-time arguments object will refer to the same frame as the run-time object.

Security-sensitive because I think you might be able to read out of bounds.

Comment 1

[Andreas Gal :gal]

Does this affect branch? I think we don't trace this case on 1.9.1.

Comment 2

[Jason Orendorff [:jorendorff]]

I think you're right; see bug 453730 for the history.

Comment 3

[David Mandelin [:dmandelin]]

This is not on 1.9.1.

The cause of the bug is that the LIR recorded for JSOP_GETELEM on a js_Arguments assumes you will always get an arguments object for the "same" frame later on. Currently, the arguments object created on trace doesn't even carry any information to say what frame it refers to.

This is going to be tricky. I can definitely put some kind of cookie into the private field of arguments objects I create on trace, and then call a builtin to read properties that refers to that cookie, similarly to GetUpvarOnTrace. But a trace can always encounter an arguments object that was created by the interpreter and imported into the trace, and I need a way to distinguish.

One option is to convert all arguments objects from the one format to the other when entering or leaving trace. It would be sucky to scan all the classes of all the objects imported. I guess it would also work to scan all the fp->argsobj of active frames, but even that seems like a lot.

Another option is to use a tag bit on the private pointer (I think 2 bits are available) to say what kind of object it is. That might work out OK. I'll need to think about it more.

Comment 4

[David Mandelin [:dmandelin]]

Attachment: Patch

After thinking about it while walking around some park for a bit, I realized that it would be pretty easy to write a guard that ensures that the arguments object is in fact always for the "same" [1] frame. This should still trace all common uses of arguments, and is much less complex than the solutions I discussed in previous comments.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 392625 [details] [diff] [review]
Patch

r=me with a significant change: On IRC Graydon asked that we hold up re-indenting all that code until the NJ merge is complete. The way it's re-indented is buggy anyway.

Comment 6

[David Mandelin [:dmandelin]]

Pushed to TM without reindent as 0b279dbc6d90.

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0b279dbc6d90

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0b279dbc6d90

Comment 9

[Bob Clary [:bc] (inactive)]

test in js/src/trace-test.js

Comment 10

[Bob Clary [:bc] (inactive)]

testMultipleArgumentsObjects
v 1.9.3, 1.9.2