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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase for js shell

Found this in browser, but reduced to a shell testcase.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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();

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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).

Comment 3

[Mike Shaver (:shaver emeritus)]

I don't see for..in in the reduced test case -- is it testing something else, or should we tweak the summary?

Comment 4

[Mike Shaver (:shaver emeritus)]

Requires setting __proto__, so not blocking, but we'd take it.

Comment 5

[Andreas Gal :gal]

Has nothing to do with for-in. The walk-up-the-proto-chain code is wrong for the array-like access.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Setting obj.__proto__ must regenerate the shape of obj, else fixed-distance walks up the proto-chain are unsound

Comment 7

[Brendan Eich [:brendan]]

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

Comment 8

[Jeff Walden [:Waldo]]

(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

Comment 9

[Jeff Walden [:Waldo]]

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...

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/56f77c5d4b89

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 12

[Brendan Eich [:brendan]]

Waldo: does the 1.9.1 branch need a refreshed/merged patch?

/be

Comment 13

[Jeff Walden [:Waldo]]

Hrm...

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/692408b49524

pushlog sez it was a sayrer-merge at Thu Jun 04 23:32:33 2009 -0700:

http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?startdate=Jun+04+2009+23%3A30&enddate=Jun+04+2009+23%3A37

Comment 14

[Damon Sicore (:damons)]

Would have blocked 1.9.2.  Marking as such.

Comment 15

[Robert Sayre]

fixed before we branched 1.9.2

Comment 16

[Bob Clary [:bc] (inactive)]

js/src/trace-test.js

Comment 17

[Bob Clary [:bc] (inactive)]

testSetProtoRegeneratesObjectShape in trace-test.js
v 1.9.3, 1.9.2