632901 - TM: crash when assigning to function.arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

function f(o) {
    var prop = "arguments";
    f[prop] = f[prop];
}
for(var i=0; i<10; i++) {
    f();
}
--
This crashes with -j in debug and release builds.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Stacktrace

Comment 2

[David Mandelin [:dmandelin]]

This is probably fairly easy, let's fix. js_PutArgumentsOnTrace gets called from trace with a NULL argsobj.

Comment 3

[Nicholas Nethercote [inactive]]

Here's what I found out:

- The js_PutArgumentsOnTrace call is generated in putActivationObjects().

- The call is generated because |haveArgs| is true, which seems reasonable,
  because |f| does have/use |f.arguments|.

- This call:

    argsobj_ins = getFrameObjPtr(fp->addressOfArgs());

  returns a LIR immediate instruction whose value is zero.  getFrameObjPtr()
  calls getImpl() which calls getFromTrackerImpl() which succeeds with the
  zero-immediate instruction.  AFAICT this is because record_EnterFrame() does
  this:

    // argsObj: clear and set to null
    nativeFrameTracker.set(fp->addressOfArgs(), NULL);
    setFrameObjPtr(fp->addressOfArgs(), w.immpNull());

- If the bytecode had contained a JSOP_ARGUMENTS, then this would happen in
  record_JSOP_ARGUMENTS:

    setFrameObjPtr(fp->addressOfArgs(), args_ins);

  But the bytecode doesn't contain JSOP_ARGUMENTS, it looks like this:

00000:   2  string "arguments"
00003:   2  setlocal 0
00006:   2  pop
00007:   3  getglobal "f"
00010:   3  getlocal 0
00013:   3  getglobal "f"
00016:   3  getlocal 0
00019:   3  getelem
00020:   3  setelem
00021:   3  pop
00022:   3  stop

So the problem is that we're do various special things to handle |arguments|,
but in this case the tracer fails to realize it's dealing with |arguments|
due to the indirectness of the access, and so it doesn't do the special 
things.  Bleh.

Maybe we could abort tracing if GETELEM's index is "arguments".  But you
could imagine cases where the record-time index is not "arguments" but
a later one is.  Hmm.

Comment 4

[Nicholas Nethercote [inactive]]

(Oh: this totally shouldn't be a hardblocker, IMO.)

Comment 5

[Nicholas Nethercote [inactive]]

dvander and Waldo point out that |f.arguments| is bogus, BTW.

Comment 6

[Brendan Eich [:brendan]]

We do know that the base object of the getelem is a function (distinct trace type). This tells us enough to know we should guard on the indexed id not being 'arguments', as well as abort recording for that id.

/be

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 61866:85610aaf53cc)

This patch implements dvander's idea of aborting if have_args is true but script->usesArguments is false.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 511288 [details] [diff] [review]
patch (against TM 61866:85610aaf53cc)

Could the record-time check not be enough? What if the runtime property name is "arguments" but the record time is something else? Something like

function f(o, prop) {
    f[prop] = f[prop];
}
var a=[
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "Arguments",
    "arguments",
];
for(var i=0; i<10; i++) {
    f(undefined, a[i]);
}

but I'm out of time to try to make this fail right now. feedback- without a proof that trace runtime can't mismatch record time due to an existing guard guarantee. If there is a proof, great -- put it in a comment.

/be

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Brendan, it should be okay. In this case we wouldn't have emitted the call to js_PutArgsOnTrace, and js_GetArgsValue will deep bail asking for the top JSStackFrame. The bug in comment #0 is that we missed the js_CreateArgsOnTrace call, the attached patch should account for that (maybe there is a more explicit way though?)

Comment 10

[Daniel Veditz [:dveditz]]

Is this a recent regression? My somewhat old trunk js shell doesn't crash.

Is there something other than a null-pointer crash going on here?

Comment 11

[Nicholas Nethercote [inactive]]

So the patch has r+ from dvander and f- from Brendan.  Brendan, are you convinced by comment 9?

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 511288 [details] [diff] [review]
patch (against TM 61866:85610aaf53cc)

I should have left the ? standing, to avoid losing track of this. Thanks, David, that's all I wanted. Put it in a comment and r=me too.

/be

Comment 13

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/dd971b2a6129

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #10)
> Is this a recent regression? My somewhat old trunk js shell doesn't crash.

I tried it with 1.9.1 and 1.9.2 and it didn't crash with either.

> Is there something other than a null-pointer crash going on here?

Not AFAICT.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/dd971b2a6129

Comment 16

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug632901.js.