If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla21

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Norbert Lindenberg, Unassigned)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 692696 [details] [diff] [review]
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
(Reporter)

Updated

5 years ago
Blocks: 837963
(Reporter)

Updated

5 years ago
No longer blocks: 769872
(Reporter)

Updated

5 years ago
Blocks: 784288
Created attachment 714506 [details] [diff] [review]
use defineProperty instead of setGeneric during cloning of object properties for self-hosting.

Simple fix
Attachment #714506 - Flags: review?(jwalden+bmo)
Attachment #714506 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/88361e96d89f
https://hg.mozilla.org/mozilla-central/rev/88361e96d89f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Reporter)

Comment 4

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Created attachment 718692 [details] [diff] [review]
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.
Attachment #718692 - Flags: review?(mozillabugs)
Attachment #718692 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 7

5 years ago
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.
Attachment #718692 - Flags: review?(mozillabugs) → review-
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.
Attachment #718692 - Flags: review?(jwalden+bmo)
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!
(Reporter)

Updated

5 years ago
Blocks: 853704
(Reporter)

Comment 10

4 years ago
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.
Blocks: 895863
(Assignee)

Updated

3 years ago
Assignee: general → nobody
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....)
Flags: needinfo?(till)
Depends on: 1146979
And in fact, just filed bug 1146979.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago3 years ago
Flags: needinfo?(till)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.