JM: IsCacheableProtoChain must check for a null proto

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({regression})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5+ fixed, firefox6 fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

IsCacheableProtoChain from PolyIC.cpp assumes that it either finds non-native object on the prototype chain or get the holder object for the property that is returned by lookupProperty.

Hover, this does not hold for proxies. A proxy on the prototype can mutate the protptype chain under the lookupProperty call and remove itself from it. Then IsCacheableProtoChain would not find the object and crash with NULL pointer dereference. The following example demonstrates this in JS shell:

~/s> cat x.js
var obj;
var counter = 0;
var p = Proxy.create({
    has : function(id) {
	if (id == 'xyz') {
	    ++counter;
	    if (counter == 7) {
		obj.__proto__ = null;
	    }
	    return true;
	}
	return false;
    },
    get : function(id) {
	if (id == 'xyz')
	    return 10;
    }
});

function test()
{
    Object.prototype.__proto__ = null;
    obj = { xyz: 1};
    var n = 0;
    for (var i = 0; i != 100; ++i) {
	var s = obj.xyz;
	if (s)
	    ++n;
	if (i == 10) {
	    delete obj.xyz;
	    Object.prototype.__proto__ = p;
	}

    }
}

test();
~/s> ~/b/js/tmdbg/js -m x.js
Segmentation fault

The bug is a regression from the bug 602641. Prior the changes there the code contained NULL property checks when searching the proto.
Posted patch v1Splinter Review
adding a null-pointer check and a test
Assignee: general → igor
Attachment #532620 - Flags: review?(dvander)
Attachment #532620 - Flags: review?(dvander) → review+
http://hg.mozilla.org/tracemonkey/rev/90a5b6161f6d
Whiteboard: fixed-in-tracemonkey
Nominating for ff 5.0 as it fixes a NULL dereference
http://hg.mozilla.org/tracemonkey/rev/50a73415c4b6 - followup to fix the regression test not to throw an exception
Igor, rather than nominating this to be tracking-firefox5, please just request approval-mozilla-beta on the patch itself. Thanks.
Attachment #532620 - Flags: approval-mozilla-beta?
jit-test/tests/basic/bug657227.js is failing for me, with 0621dbdec4c2. Is that to be expected? This is on amd64 linux.
(In reply to comment #6)
> jit-test/tests/basic/bug657227.js is failing for me, with 0621dbdec4c2. Is
> that to be expected? This is on amd64 linux.

How it fails?
djc@miles src $ python jit-test/jit_test.py --show-output dist/bin/js basic/bug657227.jsExit code: -11
[   0|   1|   1] 100% ===============================================>|    0.0s
FAILURES:
    -m -j -p /home/djc/src/tracemonkey/js/src/jit-test/tests/basic/bug657227.js
(In reply to comment #8)
> djc@miles src $ python jit-test/jit_test.py --show-output dist/bin/js
> basic/bug657227.jsExit code: -11

So you have not get any output? If you just run ...dist/bin/js -m bug657227.js directly, what would happen?
Ah, sorry about that. It was a segfault, and it passes after rebuilding.
Attachment #532620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 532620 [details] [diff] [review]
v1

Please land this change on both Aurora and Beta. (In the future, getting changes in during Aurora will save you this extra step.)
Attachment #532620 - Flags: approval-mozilla-aurora+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> Comment on attachment 532620 [details] [diff] [review] [review]
> v1
> 
> Please land this change on both Aurora and Beta. (In the future, getting
> changes in during Aurora will save you this extra step.)

Landing ping: Igor, could you please land this on mozilla-aurora and mozilla-beta?
(In reply to comment #13)
> Landing ping: Igor, could you please land this on mozilla-aurora and
> mozilla-beta?

Hm, the patch is already in aurora:

http://hg.mozilla.org/mozilla-aurora/rev/90a5b6161f6d
http://hg.mozilla.org/mozilla-aurora/rev/50a73415c4b6

So should I land this only on beta?
(In reply to comment #14)
> (In reply to comment #13)
> > Landing ping: Igor, could you please land this on mozilla-aurora and
> > mozilla-beta?
> 
> Hm, the patch is already in aurora:
> 
> http://hg.mozilla.org/mozilla-aurora/rev/90a5b6161f6d
> http://hg.mozilla.org/mozilla-aurora/rev/50a73415c4b6
> 
> So should I land this only on beta?

Yes. (I just saw the approval flag for aurora and I didn't check to see if it was already there.)
Attachment #532620 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.