657227 - JM: IsCacheableProtoChain must check for a null proto

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

Attachment: v1

adding a null-pointer check and a test

Comment 2

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/90a5b6161f6d

Comment 3

[Igor Bukanov]

Nominating for ff 5.0 as it fixes a NULL dereference

Comment 4

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/50a73415c4b6 - followup to fix the regression test not to throw an exception

Comment 5

[Asa Dotzler [:asa]]

Igor, rather than nominating this to be tracking-firefox5, please just request approval-mozilla-beta on the patch itself. Thanks.

Comment 6

[Dirkjan Ochtman (:djc)]

jit-test/tests/basic/bug657227.js is failing for me, with 0621dbdec4c2. Is that to be expected? This is on amd64 linux.

Comment 7

[Igor Bukanov]

(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?

Comment 8

[Dirkjan Ochtman (:djc)]

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

Comment 9

[Igor Bukanov]

(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?

Comment 10

[Dirkjan Ochtman (:djc)]

Ah, sorry about that. It was a segfault, and it passes after rebuilding.

Comment 11

[Asa Dotzler [:asa]]

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

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/90a5b6161f6d
http://hg.mozilla.org/mozilla-central/rev/50a73415c4b6

Comment 13

[David Mandelin [:dmandelin]]

(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?

Comment 14

[Igor Bukanov]

(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?

Comment 15

[David Mandelin [:dmandelin]]

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

Comment 16

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-beta/rev/7110ab92b835