822080 - Cloning of variables in self-hosted code can be observed by client code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

Attachment: Test case showing that cloning of top-level variables in self-hosted code can be observed. Do not check in.

The fix for bug 784293 has a problem: The copying of properties of top-level variables within self-hosted is visible to client software.

To reproduce, apply the attached patch, build, and run "js spy.js". With the current fix for bug 784293, the client code in spy.js has access to the properties of the self-hosted variable.

That shouldn't happen - in spec terminology, cloning should "define" properties, not "assign" them.
http://www.2ality.com/2012/08/property-definition-assignment.html

Comment 1

[Till Schneidereit [:till]]

Attachment: use defineProperty instead of setGeneric during cloning of object properties for self-hosting.

Simple fix

Comment 2

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88361e96d89f

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88361e96d89f

Comment 4

[Norbert Lindenberg]

The fix that was checked in is not correct: It can change the attributes of properties. It caused two new failures in the ECMA-402 conformance test suite (while fixing 7 old ones). The new failures occurred because a for-in loop in InitializeCollator doesn't find any properties in the collatorKeyMappings variable anymore, because the "enumerable" attribute on the properties was set to false during cloning.

A simple fix for these failures is to replace the last argument for the new JS_DefinePropertyById call with JSPROP_ENUMERATE. I'm not sure this is sufficient though - it seems a complete fix would have to clone all attributes.

Comment 5

[Jeff Walden [:Waldo]]

Hmm, right -- all attributes do indeed have to be cloned.  Another demonstration of how the current cloning thing is pretty fragile, and not really the right solution.  :-\  Not that I have time to implement something better, nor ideas about exactly how to make it nicer, tho.

Comment 6

[Till Schneidereit [:till]]

Attachment: copy attributes of cloned properties during self-hosted value cloning. r=?

Argh, I could've sworn I attached this patch a week ago. I might have mis-formatted something in hg bzexport and not checked for the result. Sorry for the delay.

Anyway, I agree with waldo, but don't have any clever ideas for not-cloning-everything, either. In light of that, here's a new attempt to fix this. Again trivial.

Comment 7

[Norbert Lindenberg]

Comment on attachment 718692 [details] [diff] [review]
copy attributes of cloned properties during self-hosted value cloning. r=?

Review of attachment 718692 [details] [diff] [review]:
-----------------------------------------------------------------

I can't find documentation on JSObject::getGenericAttributes, so I'll leave comments on that to Jeff.

However, I tested the patch, and it causes almost all my test cases to fail.

To test for yourself:
- Pull the latest sources so that you have my patches up to part 11 of bug 769872.
- Add the patches for parts 12-16 of that bug.
- Change the #define ENABLE_INTL_API to 1 in jsversion.h.
- Build.
- Download Test262 - see http://wiki.ecmascript.org/doku.php?id=test262:command
- Run the tests for Collator, that is, use intl402/ch10 as {{path}}
- If all went well so far, you'll see three failures and 29 tests passing.
- Add attrs.patch, rebuild.
- Run the tests again; at this point they'll all fail.

The test failures introduced by the previous fix are the first two in the Collator set, intl402/ch10/10.1/10.1.1_1 and intl402/ch10/10.1/10.1.1_19_c, so they're the ones you really want to get to pass.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 718692 [details] [diff] [review]
copy attributes of cloned properties during self-hosted value cloning. r=?

I'm totally guessing, but I'm betting |attrs| includes extra flags that need to be masked out here.

Comment 9

[Till Schneidereit [:till]]

Hrmpf, sorry. I'll look into this over the next few days and attach a new patch. I might even actually test that one, we'll see!

Comment 10

[Norbert Lindenberg]

GDB shows that the crashes are actually within the new call to JSObject::getGenericAttributes, and relate to a compartment mismatch:

js> this.hasOwnProperty("Intl")
*** Compartment mismatch 0x102844600 vs. 0x102849800
Hit MOZ_CRASH() at /Users/standards/mozilla/intl/js/src/jscntxtinlines.h:144

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000
js::CompartmentChecker::fail (c1=0x102844600, c2=0x102849800) at jscntxtinlines.h:144
144	        MOZ_CRASH();
(gdb) bt
#0  js::CompartmentChecker::fail (c1=0x102844600, c2=0x102849800) at jscntxtinlines.h:144
#1  0x00000001002e9f01 in js::CompartmentChecker::check (this=0x7fff5fbecb50, c=0x102849800) at jscntxtinlines.h:165
#2  0x00000001001d46f5 in js::assertSameCompartmentDebugOnly<JSCompartment*> (cx=0x10220b8f0, t1=@0x7fff5fbecbf0) at jscntxtinlines.h:259
#3  0x00000001001cc37d in js::Shape::makeOwnBaseShape (this=0x102623768, cx=0x10220b8f0) at /Users/standards/mozilla/intl/js/src/vm/Shape.cpp:72
#4  0x00000001001d499b in js::Shape::ensureOwnBaseShape (this=0x102623768, cx=0x10220b8f0) at Shape.h:543
#5  0x00000001001cc732 in js::Shape::hashify (cx=0x10220b8f0, shape=0x102623768) at /Users/standards/mozilla/intl/js/src/vm/Shape.cpp:111
#6  0x00000001001b3914 in js::Shape::search (cx=0x10220b8f0, start=0x102623768, id={asBits = 4332902592}, pspp=0x7fff5fbecd30, adding=false) at Shape.h:1079
#7  0x0000000100131cc5 in js::ObjectImpl::nativeLookup (this=0x1026032e0, cx=0x10220b8f0, id={asBits = 4332902592}) at /Users/standards/mozilla/intl/js/src/vm/ObjectImpl.cpp:329
#8  0x000000010042aafb in LookupPropertyWithFlagsInline (cx=0x10220b8f0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbed2c0}, flags=65535, objp={<js::MutableHandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed028}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbed000}) at /Users/standards/mozilla/intl/js/src/jsobj.cpp:3683
#9  0x0000000100438a3f in js::baseops::LookupProperty<(js::AllowGC)1> (cx=0x10220b8f0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbed2c0}, objp={<js::MutableHandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed028}, propp={<js::MutableHandleBase<js::Shape *>> = {<No data fields>}, ptr = 0x7fff5fbed000}) at /Users/standards/mozilla/intl/js/src/jsobj.cpp:3752
#10 0x000000010042f716 in js::baseops::GetAttributes (cx=0x10220b8f0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbed2c0}, attrsp=0x7fff5fbed1c4) at /Users/standards/mozilla/intl/js/src/jsobj.cpp:4528
#11 0x00000001000206b8 in JSObject::getGenericAttributes (cx=0x10220b8f0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbed2c0}, attrsp=0x7fff5fbed1c4) at jsobj.h:958
#12 0x00000001001c7272 in CloneProperties (cx=0x10220b8f0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, clone={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed530}, clonedObjects=@0x7fff5fbed790) at /Users/standards/mozilla/intl/js/src/vm/SelfHosting.cpp:760
#13 0x00000001001c6dda in CloneObject (cx=0x10220b8f0, srcObj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbed6f0}, clonedObjects=@0x7fff5fbed790) at /Users/standards/mozilla/intl/js/src/vm/SelfHosting.cpp:826

The attrs values occurring before the crash are either 1 (JSPROP_ENUMERATE) or 7 (JSPROP_ENUMERATE + JSPROP_READONLY + JSPROP_PERMANENT) - nothing exotic.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I ran into this bug today: I was trying to return an object from self-hosted code and it came out with all properties non-enumerable.

Why can't we just use shape->attributes() of the relevant shape?

(As a note, comment 4 should _really_ have been a new bug blocking this one....)

Comment 12

[Boris Zbarsky [:bzbarsky]]

And in fact, just filed bug 1146979.