Closed Bug 632901 Opened 9 years ago Closed 9 years ago

TM: crash when assigning to function.arguments

Categories

(Core :: JavaScript Engine, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- .x+

People

(Reporter: jandem, Unassigned)

References

Details

(4 keywords, Whiteboard: [sg:needinfo], fixed-in-tracemonkey)

Attachments

(2 files)

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.
Severity: normal → critical
blocking2.0: --- → ?
Attached file Stacktrace
This is probably fairly easy, let's fix. js_PutArgumentsOnTrace gets called from trace with a NULL argsobj.
blocking2.0: ? → final+
Whiteboard: hardblocker
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.
(Oh: this totally shouldn't be a hardblocker, IMO.)
dvander and Waldo point out that |f.arguments| is bogus, BTW.
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
This patch implements dvander's idea of aborting if have_args is true but script->usesArguments is false.
Attachment #511288 - Flags: review?(dvander)
Attachment #511288 - Flags: feedback?(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
Attachment #511288 - Flags: feedback?(brendan) → feedback-
blocking2.0: final+ → .x
Whiteboard: hardblocker
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?)
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?
Keywords: regression
Whiteboard: [sg:needinfo]
Attachment #511288 - Flags: review?(dvander) → review+
So the patch has r+ from dvander and f- from Brendan.  Brendan, are you convinced by comment 9?
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
Attachment #511288 - Flags: feedback- → feedback+
Attachment #511288 - Flags: approval2.0?
Attachment #511288 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/tracemonkey/rev/dd971b2a6129
Whiteboard: [sg:needinfo] → [sg:needinfo], fixed-in-tracemonkey
(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.
Group: core-security
http://hg.mozilla.org/mozilla-central/rev/dd971b2a6129
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug632901.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.