TM: "Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE)," or "Assertion failure: hasInt32Repr(*(const Value *)p),"

VERIFIED FIXED

Status

()

defect
--
critical
VERIFIED FIXED
9 years ago
4 years ago

People

(Reporter: gkw, Assigned: dvander)

Tracking

(Blocks 1 bug, {assertion, regression, testcase})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical][hardblocker][fixed-in-tracemonkey])

Attachments

(1 attachment, 2 obsolete attachments)

for (z in [0])
function b() {
    Object.defineProperty(this, "z", ({
        value: this
    }))
    this.z = 1e81
}
for (a in [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) {
    try {
        b()
    } catch (e) {}
}

asserts js debug shell on TM changeset 3559d9fded5f with -j at Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE),
Here's a similar variant but with a different assert message.

function b() {
    Object.defineProperty(this, "z", ({
        value: this,
        writable: true
    }));
    z = 1
}
for (a in [0, 0, 0, 0, 0, 0, 0, 0, 0, 0]) {
    try {
        b()
    } catch (e) {}
}


Assertion failure: hasInt32Repr(*(const Value *)p),
blocking2.0: --- → ?
Summary: TM: "Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE)," → TM: "Assertion failure: ((const Value *)p)->isNumber() == (t == JSVAL_TYPE_DOUBLE)," or "Assertion failure: hasInt32Repr(*(const Value *)p),"
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   36651:766a6b2e74e7
user:        Jeff Walden
date:        Fri Jun 05 12:56:45 2009 -0700
summary:     Bug 430133 - Implement ES3.1's Object.defineProperty and
Object.defineProperties.  r=jorendorff
Blocks: 430133
Fix coming.
Assignee: general → jorendorff
I thought this would be trivial to fix, but it's not. Abandoning.

Here's the minimized test:

z = 0;
for (var i = 0; i < 20; i++) {
    Object.defineProperty(this, "z", {value: ""});
    z = 1;
}

It asserts the second time we try to trace this loop. On loop entry, the TraceRecorder knows that global.z is an int32. Object.defineProperty quietly stores a string in the global slot for z. The tracer doesn't notice that this has happened. So when we execute z=1, the TraceRecorder tries to import global.z using the type from the importTypeMap, which is wrong.
I think if we went through and recorded this trace, we would just bail off it (safely) at run time. In this case.

A possible solution is for LeaveTraceIfGlobalObject to check for a TraceRecorder and abort recording if any is present; but dvander points out that this might be too pessimistic. The interpreter takes some slow paths, possibly in cases where the tracer knows exactly what's going on. We don't want to abort in those cases.

So, another possible solution is for LeaveTraceIfGlobalObject to set a flag in the TraceRecorder (if any) which tells it to re-check the global slot types it thinks it knows. The TraceRecorder would have to check this after every time the interpreter has a chance to do unexpected stuff.
Assignee: jorendorff → general
Group: core-security
In bug 561954 dvander suggests aborting.
> I think if we went through and recorded this trace, we would just bail off it
> (safely) at run time. In this case.

This is definitely true. You can arrange for Object.defineProperty to do nothing at record time, then do something nefarious only on trace. In that case we correctly leave trace every time.

var harmless = {value: 0};
var bad = {value: ""};
var a = [bad, bad, bad, bad, bad, bad, bad, bad, harmless, bad, bad, bad];

z = 0;
for (var i = 0; i < a.length; i++) {
    print(tracemonkey.onTrace);
    Object.defineProperty(this, "z", a[i]);
    z = 1;
}
Aborting is really the way to go whenever the global object is manipulated behind the back of the tracer.
blocking2.0: ? → betaN+
Whiteboard: [sg:critical]
This bug seems fixable and needs an assignee. Rob, can you find a volunteer?

/be
Assignee: general → jwalden+bmo
waldo, any updates here?
No, I haven't had time to look at this.  :-(
Jeff, any idea when you'll get to this?
Sooner, now that bug 514568 is wrapping up.  Not sure about an exact time yet, though, but I hope before end of year, if other blockers cooperate.
Whiteboard: [sg:critical] → [sg:critical][hardblocker]
Assignee: jwalden+bmo → dvander
Status: NEW → ASSIGNED
Posted patch fix (obsolete) — Splinter Review
Thanks to Luke's work in bug 561954 all this needs is a tweak.
Attachment #501545 - Flags: review?(lw)
Explanation: This check was trying to avoid aborting the trace when setting an unexpected, but untracked global. Checking if the global slot is in the recorder's tracker isn't quite enough, since it could also be in the import type map. We could check that as well, but this case never hits on SS, v8, or Kraken, and it seems very unlikely in general.
Comment on attachment 501545 [details] [diff] [review]
fix

Thanks for doing it right :)
Attachment #501545 - Flags: review?(lw) → review+
http://hg.mozilla.org/tracemonkey/rev/48a2416b7f41
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][fixed-in-tracemonkey]
What a mess. Backed out as http://hg.mozilla.org/tracemonkey/rev/a65393138ec4.

Pretty much every fallible function in the engine can deep-abort the recorder. bug 561954 made it more likely, and this one made it extremely likely (since lazy resolving happens quite often). This isn't safe, since very few places in the recorder check to see if a fallible function caused a deep abort.
Whiteboard: [sg:critical][hardblocker][fixed-in-tracemonkey] → [sg:critical][hardblocker]
Posted patch v2 (obsolete) — Splinter Review
Attachment #501545 - Attachment is obsolete: true
Attachment #501809 - Flags: review?(lw)
Posted patch v2Splinter Review
Attachment #501809 - Attachment is obsolete: true
Attachment #501810 - Flags: review?(lw)
Attachment #501809 - Flags: review?(lw)
Comment on attachment 501810 [details] [diff] [review]
v2

>+             * Otherwise, only abort if the global is not present in the
>+             * import typemap. Just deep aborting false here is not acceptable,
>+             * because the recorder does not guard on every operation that
>+             * could lazily resolve. Since resolving adds properties to
>+             * reserved slots, the tracer will never have imported them.

Yes, this is the comment I should have written the first time, sorry.  (Except, isn't it "only abort if the global is present in the import typemap." ?)
Attachment #501810 - Flags: review?(lw) → review+
http://hg.mozilla.org/tracemonkey/rev/d8586631c5f0
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][fixed-in-tracemonkey]
follow-up fix for assert botch, limit the global checks to only the global the trace is keyed on: http://hg.mozilla.org/tracemonkey/rev/d08f63692c60
http://hg.mozilla.org/mozilla-central/rev/d08f63692c60
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
JSBugMon: This bug has been automatically verified fixed.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.