Last Comment Bug 657227 - JM: IsCacheableProtoChain must check for a null proto
: JM: IsCacheableProtoChain must check for a null proto
Status: RESOLVED FIXED
fixed-in-tracemonkey
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 602641
  Show dependency treegraph
 
Reported: 2011-05-15 10:34 PDT by Igor Bukanov
Modified: 2011-08-03 16:36 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed


Attachments
v1 (1.63 KB, patch)
2011-05-16 06:23 PDT, Igor Bukanov
dvander: review+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-05-15 10:34:17 PDT
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 2011-05-16 06:23:15 PDT
Created attachment 532620 [details] [diff] [review]
v1

adding a null-pointer check and a test
Comment 3 Igor Bukanov 2011-05-18 05:57:11 PDT
Nominating for ff 5.0 as it fixes a NULL dereference
Comment 4 Igor Bukanov 2011-05-18 06:38:49 PDT
http://hg.mozilla.org/tracemonkey/rev/50a73415c4b6 - followup to fix the regression test not to throw an exception
Comment 5 Asa Dotzler [:asa] 2011-05-19 00:07:08 PDT
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) 2011-05-19 10:50:27 PDT
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 2011-05-19 11:08:24 PDT
(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) 2011-05-19 11:18:38 PDT
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 2011-05-19 11:43:58 PDT
(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) 2011-05-19 13:28:57 PDT
Ah, sorry about that. It was a segfault, and it passes after rebuilding.
Comment 11 Asa Dotzler [:asa] 2011-05-19 15:16:58 PDT
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) 2011-05-23 14:15:54 PDT
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] 2011-05-24 18:07:32 PDT
(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 2011-05-25 04:48:01 PDT
(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] 2011-05-25 08:40:08 PDT
(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.)

Note You need to log in before you can comment on or make changes to this bug.