Closed Bug 496054 Opened 13 years ago Closed 13 years ago

TM: Null deref [@ JITted code] involving __proto__ munging and array-like access

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
status1.9.1 --- ?

People

(Reporter: jruderman, Assigned: Waldo)

References

Details

(5 keywords, Whiteboard: fixed-in-tracemonkey)

Crash Data

Attachments

(2 files)

Attached file testcase for js shell
Found this in browser, but reduced to a shell testcase.
Attachment #381208 - Attachment mime type: text/javascript → text/plain
Slightly reduced testcase by gal, accidentally wrongly posted in bug 495958 comment 9 :


function boom()
{
    var f = function(){};
    var g = function(){};
    g.prototype.__proto__ = {};
    iq(f);
    iq(f);
    iq(f);
    iq(f);
    iq(g);
}

function iq(obj)
{
    for (var i = 0; i < 10; ++i)
    "" + obj["prototype"];
}

boom();
autoBisect shows this is probably related to bug 463238 :

The first bad revision is:
changeset:   26344:1c6be1c210b9
user:        Andreas Gal
date:        Fri Mar 20 18:52:11 2009 -0700
summary:     Support calling arbitrary JSFastNatives from trace (463238, r=brendan).
Blocks: 463238
Flags: blocking1.9.1?
Keywords: regression
I don't see for..in in the reduced test case -- is it testing something else, or should we tweak the summary?
Requires setting __proto__, so not blocking, but we'd take it.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Flags: blocking1.9.2?
Has nothing to do with for-in. The walk-up-the-proto-chain code is wrong for the array-like access.
Summary: TM: Null deref [@ JITted code] involving for..in, __proto__ → TM: Null deref [@ JITted code] involving __proto__ munging and array-like access
Assignee: general → jwalden+bmo
Comment on attachment 381417 [details] [diff] [review]
Setting obj.__proto__ must regenerate the shape of obj, else fixed-distance walks up the proto-chain are unsound

Could minimize the patch by changing the oldproto initializer to obj. Kind of works. Worth it for end-game hygiene?

The revised comment doesn't wrap as nicely, which makes the minimization more attractive.

r=me with these thoughts addressed somehow.

/be
Attachment #381417 - Flags: review?(brendan) → review+
(In reply to comment #7)
> Could minimize the patch by changing the oldproto initializer to obj. Kind of
> works. Worth it for end-game hygiene?

Works well enough, not a whole lot of win here anyway, reverted to the one-liner.

http://hg.mozilla.org/tracemonkey/rev/56f77c5d4b89
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: fixed-in-tracemonkey
Comment on attachment 381417 [details] [diff] [review]
Setting obj.__proto__ must regenerate the shape of obj, else fixed-distance walks up the proto-chain are unsound

(Note: approval request is on what was pushed, which addressed the review comments and minimized this to a one-liner.)

Small change, easily understood, attachment description says all, regenerating shapes should always be safe anyway...
Attachment #381417 - Flags: approval1.9.1?
http://hg.mozilla.org/mozilla-central/rev/56f77c5d4b89
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 381417 [details] [diff] [review]
Setting obj.__proto__ must regenerate the shape of obj, else fixed-distance walks up the proto-chain are unsound

removing dead flag; please generate a new mozilla-1.9.1 patch and request approval on that.
Waldo: does the 1.9.1 branch need a refreshed/merged patch?

/be
Would have blocked 1.9.2.  Marking as such.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
fixed before we branched 1.9.2
js/src/trace-test.js
Flags: in-testsuite+
testSetProtoRegeneratesObjectShape in trace-test.js
v 1.9.3, 1.9.2
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
Crash Signature: [@ JITted code]
You need to log in before you can comment on or make changes to this bug.