The default bug view has changed. See this FAQ.

JM: IsCacheableProtoChain must check for a null proto

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({regression})

unspecified
regression
Points:
---

Firefox Tracking Flags

(firefox5+ fixed, firefox6 fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 532620 [details] [diff] [review]
v1

adding a null-pointer check and a test
Assignee: general → igor
Attachment #532620 - Flags: review?(dvander)
Attachment #532620 - Flags: review?(dvander) → review+
(Assignee)

Comment 2

6 years ago
http://hg.mozilla.org/tracemonkey/rev/90a5b6161f6d
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 3

6 years ago
Nominating for ff 5.0 as it fixes a NULL dereference
tracking-firefox5: --- → ?
(Assignee)

Comment 4

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

Comment 5

6 years ago
Igor, rather than nominating this to be tracking-firefox5, please just request approval-mozilla-beta on the patch itself. Thanks.
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 7

6 years ago
(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
(Assignee)

Comment 9

6 years ago
(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.

Updated

6 years ago
Attachment #532620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: regression

Comment 11

6 years ago
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+
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/90a5b6161f6d
http://hg.mozilla.org/mozilla-central/rev/50a73415c4b6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
tracking-firefox5: ? → +
(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?
(Assignee)

Comment 14

6 years ago
(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.)
(Assignee)

Comment 16

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/7110ab92b835
status-firefox5: --- → fixed

Updated

6 years ago
Attachment #532620 - Flags: approval-mozilla-aurora+

Updated

6 years ago
status-firefox6: --- → fixed
You need to log in before you can comment on or make changes to this bug.