From 516b2a2a71ea4f6dbb38a8fe7d7d6e305628c2ec Mon Sep 17 00:00:00 2001 From: Matthew Gaudet Date: Mon, 20 Jul 2020 13:49:12 +0000 Subject: [PATCH] Bug 1644160 - Initial Proxy Support for Private Fields r=jorendorff Differential Revision: https://phabricator.services.mozilla.com/D83145 --- js/public/Proxy.h | 24 ++++ .../tests/fields/private-proxy-oom.js | 48 ++++++++ js/src/proxy/BaseProxyHandler.cpp | 107 ++++++++++++++++++ js/src/proxy/Proxy.cpp | 54 +++++++++ js/src/tests/non262/PrivateName/proxy-1.js | 21 ++++ .../non262/PrivateName/proxy-init-set.js | 75 ++++++++++++ js/src/vm/ProxyObject.cpp | 16 +++ js/src/vm/ProxyObject.h | 8 ++ 8 files changed, 353 insertions(+) create mode 100644 js/src/jit-test/tests/fields/private-proxy-oom.js create mode 100644 js/src/tests/non262/PrivateName/proxy-1.js create mode 100644 js/src/tests/non262/PrivateName/proxy-init-set.js diff --git a/js/public/Proxy.h b/js/public/Proxy.h index ce86f0c975ea..f57ca13879d6 100644 --- a/js/public/Proxy.h +++ b/js/public/Proxy.h @@ -317,6 +317,22 @@ class JS_FRIEND_API BaseProxyHandler { HandleValue v, HandleValue receiver, ObjectOpResult& result) const; + // Private fields don't use [[Has]], [[Get]] and [[Set]] semantics, as they + // technically use a weak-map semantics, however we end up on these paths with + // our implementation. + virtual bool hasPrivate(JSContext* cx, HandleObject proxy, HandleId id, + bool* bp) const; + virtual bool getPrivate(JSContext* cx, HandleObject proxy, + HandleValue receiver, HandleId id, + MutableHandleValue vp) const; + virtual bool setPrivate(JSContext* cx, HandleObject proxy, HandleId id, + HandleValue v, HandleValue receiver, + ObjectOpResult& result) const; + + virtual bool definePrivateField(JSContext* cx, HandleObject proxy, + HandleId id, Handle desc, + ObjectOpResult& result) const; + /* * [[Call]] and [[Construct]] are standard internal methods but according * to the spec, they are not present on every object. @@ -386,6 +402,8 @@ namespace detail { // // Every proxy has a ProxyValueArray that contains the following Values: // +// - The expando slot. This is used to hold private fields should they be +// stamped into a non-forwarding proxy type. // - The private slot. // - The reserved slots. The number of slots is determined by the proxy's Class. // @@ -419,10 +437,12 @@ struct ProxyReservedSlots { }; struct ProxyValueArray { + Value expandoSlot; Value privateSlot; ProxyReservedSlots reservedSlots; void init(size_t nreserved) { + expandoSlot = JS::ObjectOrNullValue(nullptr); privateSlot = JS::UndefinedValue(); reservedSlots.init(nreserved); } @@ -505,6 +525,10 @@ inline const Value& GetProxyPrivate(const JSObject* obj) { return detail::GetProxyDataLayout(obj)->values()->privateSlot; } +inline const Value& GetProxyExpando(const JSObject* obj) { + return detail::GetProxyDataLayout(obj)->values()->expandoSlot; +} + inline JSObject* GetProxyTargetObject(JSObject* obj) { return GetProxyPrivate(obj).toObjectOrNull(); } diff --git a/js/src/jit-test/tests/fields/private-proxy-oom.js b/js/src/jit-test/tests/fields/private-proxy-oom.js new file mode 100644 index 000000000000..75e63b78649e --- /dev/null +++ b/js/src/jit-test/tests/fields/private-proxy-oom.js @@ -0,0 +1,48 @@ +// |jit-test| skip-if: !('oomTest' in this); --enable-private-fields +// Check for proxy expando OOM issues. + +function assertThrowsTypeError(f) { + assertThrowsInstanceOf(f, TypeError); +} + + +function testing() { + var target = {}; + var p1 = new Proxy(target, {}); + var p2 = new Proxy(target, {}); + + class A extends class { + constructor(o) { + return o; + } + } + { + #field = 10; + static gf(o) { + return o.#field; + } + static sf(o) { + o.#field = 15; + } + } + + // Verify field handling on the proxy we install it on. + new A(p1); + assertEq(A.gf(p1), 10); + A.sf(p1) + assertEq(A.gf(p1), 15); + + // Should't be on the target + assertThrowsTypeError(() => A.gf(target)); + + // Can't set the field, doesn't exist + assertThrowsTypeError(() => A.sf(p2)); + + // Definitely can't get the field, doesn't exist. + assertThrowsTypeError(() => A.gf(p2)); + + // Still should't be on the target. + assertThrowsTypeError(() => A.gf(target)); +} + +oomTest(testing); \ No newline at end of file diff --git a/js/src/proxy/BaseProxyHandler.cpp b/js/src/proxy/BaseProxyHandler.cpp index 1754e1d85b54..738d79d5a333 100644 --- a/js/src/proxy/BaseProxyHandler.cpp +++ b/js/src/proxy/BaseProxyHandler.cpp @@ -22,6 +22,22 @@ bool BaseProxyHandler::enter(JSContext* cx, HandleObject wrapper, HandleId id, return true; } +bool BaseProxyHandler::hasPrivate(JSContext* cx, HandleObject proxy, + HandleId id, bool* bp) const { + assertEnteredPolicy(cx, proxy, id, GET); + + // For BaseProxyHandler, private names are stored in the expando object. + RootedObject expando(cx, proxy->as().expando().toObjectOrNull()); + + // If there is no expando object, then there is no private field. + if (!expando) { + *bp = false; + return true; + } + + return HasOwnProperty(cx, expando, id, bp); +} + bool BaseProxyHandler::has(JSContext* cx, HandleObject proxy, HandleId id, bool* bp) const { assertEnteredPolicy(cx, proxy, id, GET); @@ -69,6 +85,35 @@ bool BaseProxyHandler::hasOwn(JSContext* cx, HandleObject proxy, HandleId id, return true; } +bool BaseProxyHandler::getPrivate(JSContext* cx, HandleObject proxy, + HandleValue receiver, HandleId id, + MutableHandleValue vp) const { + assertEnteredPolicy(cx, proxy, id, GET); + + // For BaseProxyHandler, private names are stored in the expando object. + RootedObject expando(cx, proxy->as().expando().toObjectOrNull()); + + // We must have the expando, or GetPrivateElemOperation didn't call + // hasPrivate first. + MOZ_ASSERT(expando); + + // Because we controlled the creation of the expando, we know it's not a + // proxy, and so can safely call internal methods on it without worrying about + // exposing information about private names. + Rooted desc(cx); + if (!GetOwnPropertyDescriptor(cx, expando, id, &desc)) { + return false; + } + + // We must have the object, same reasoning as the expando. + MOZ_ASSERT(desc.object()); + MOZ_ASSERT(desc.hasValue()); + MOZ_ASSERT(desc.isDataDescriptor()); + + vp.set(desc.value()); + return true; +} + bool BaseProxyHandler::get(JSContext* cx, HandleObject proxy, HandleValue receiver, HandleId id, MutableHandleValue vp) const { @@ -125,6 +170,33 @@ bool BaseProxyHandler::get(JSContext* cx, HandleObject proxy, return CallGetter(cx, receiver, getterFunc, vp); } +bool BaseProxyHandler::setPrivate(JSContext* cx, HandleObject proxy, + HandleId id, HandleValue v, + HandleValue receiver, + ObjectOpResult& result) const { + assertEnteredPolicy(cx, proxy, id, SET); + MOZ_ASSERT(JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName()); + + // For BaseProxyHandler, private names are stored in the expando object. + RootedObject expando(cx, proxy->as().expando().toObjectOrNull()); + + // SetPrivateElementOperation checks for hasPrivate first, which ensures the + // expando exsists. + MOZ_ASSERT(expando); + + Rooted ownDesc(cx); + if (!GetOwnPropertyDescriptor(cx, expando, id, &ownDesc)) { + return false; + } + ownDesc.assertCompleteIfFound(); + + MOZ_ASSERT(ownDesc.object()); + + RootedValue expandoValue(cx, proxy->as().expando()); + return SetPropertyIgnoringNamedGetter(cx, expando, id, v, expandoValue, + ownDesc, result); +} + bool BaseProxyHandler::set(JSContext* cx, HandleObject proxy, HandleId id, HandleValue v, HandleValue receiver, ObjectOpResult& result) const { @@ -230,6 +302,41 @@ bool js::SetPropertyIgnoringNamedGetter(JSContext* cx, HandleObject obj, return result.succeed(); } +bool BaseProxyHandler::definePrivateField(JSContext* cx, HandleObject proxy, + HandleId id, + Handle desc, + ObjectOpResult& result) const { + MOZ_ASSERT(JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName()); + + // For BaseProxyHandler, private names are stored in the expando object. + RootedObject expando(cx, proxy->as().expando().toObjectOrNull()); + + if (!expando) { + expando = NewObjectWithGivenProto(cx, nullptr); + if (!expando) { + return false; + } + + proxy->as().setExpando(expando); + } + + // Get a new property descriptor for the expando. + Rooted ownDesc(cx); + if (!GetOwnPropertyDescriptor(cx, expando, id, &ownDesc)) { + return false; + } + ownDesc.assertCompleteIfFound(); + // Copy the incoming value into the expando descriptor. + ownDesc.setValue(desc.value()); + + // PrivateFieldAdd: Step 4: We must throw a type error if obj already has + // this property. This should have been checked by InitPrivateElemOperation + // calling hasPrivate already. + MOZ_ASSERT(!ownDesc.object()); + + return DefineProperty(cx, expando, id, ownDesc, result); +} + bool BaseProxyHandler::getOwnEnumerablePropertyKeys( JSContext* cx, HandleObject proxy, MutableHandleIdVector props) const { assertEnteredPolicy(cx, proxy, JSID_VOID, ENUMERATE); diff --git a/js/src/proxy/Proxy.cpp b/js/src/proxy/Proxy.cpp index ff2c2c1adb7d..f9763974bf66 100644 --- a/js/src/proxy/Proxy.cpp +++ b/js/src/proxy/Proxy.cpp @@ -86,6 +86,9 @@ bool Proxy::getOwnPropertyDescriptor(JSContext* cx, HandleObject proxy, if (!policy.allowed()) { return policy.returnValue(); } + + // Private names shouldn't take this path + MOZ_ASSERT_IF(JSID_IS_SYMBOL(id), !JSID_TO_SYMBOL(id)->isPrivateName()); return handler->getOwnPropertyDescriptor(cx, proxy, id, desc); } @@ -103,6 +106,17 @@ bool Proxy::defineProperty(JSContext* cx, HandleObject proxy, HandleId id, } return result.succeed(); } + + // Private field accesses have different semantics depending on the kind + // of proxy involved, and so take a different path compared to regular + // [[Get]] operations. For example, scripted handlers don't fire traps + // when accessing private fields (because of the WeakMap semantics) + bool isPrivate = JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName(); + if (isPrivate) { + return proxy->as().handler()->definePrivateField(cx, proxy, id, + desc, result); + } + return proxy->as().handler()->defineProperty(cx, proxy, id, desc, result); } @@ -135,6 +149,11 @@ bool Proxy::delete_(JSContext* cx, HandleObject proxy, HandleId id, } return ok; } + + // Private names shouldn't take this path, as deleting a private name + // should be a syntax error. + MOZ_ASSERT_IF(JSID_IS_SYMBOL(id), !JSID_TO_SYMBOL(id)->isPrivateName()); + return proxy->as().handler()->delete_(cx, proxy, id, result); } @@ -233,6 +252,9 @@ bool Proxy::has(JSContext* cx, HandleObject proxy, HandleId id, bool* bp) { return policy.returnValue(); } + // Private names shouldn't take this path, but only hasOwn; + MOZ_ASSERT_IF(JSID_IS_SYMBOL(id), !JSID_TO_SYMBOL(id)->isPrivateName()); + if (handler->hasPrototype()) { if (!handler->hasOwn(cx, proxy, id, bp)) { return false; @@ -275,6 +297,16 @@ bool Proxy::hasOwn(JSContext* cx, HandleObject proxy, HandleId id, bool* bp) { if (!policy.allowed()) { return policy.returnValue(); } + + // Private field accesses have different semantics depending on the kind + // of proxy involved, and so take a different path compared to regular + // [[Get]] operations. For example, scripted handlers don't fire traps + // when accessing private fields (because of the WeakMap semantics) + bool isPrivate = JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName(); + if (isPrivate) { + return handler->hasPrivate(cx, proxy, id, bp); + } + return handler->hasOwn(cx, proxy, id, bp); } @@ -311,6 +343,15 @@ MOZ_ALWAYS_INLINE bool Proxy::getInternal(JSContext* cx, HandleObject proxy, return policy.returnValue(); } + // Private field accesses have different semantics depending on the kind + // of proxy involved, and so take a different path compared to regular + // [[Get]] operations. For example, scripted handlers don't fire traps + // when accessing private fields (because of the WeakMap semantics) + bool isPrivate = JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName(); + if (isPrivate) { + return handler->getPrivate(cx, proxy, receiver, id, vp); + } + if (handler->hasPrototype()) { bool own; if (!handler->hasOwn(cx, proxy, id, &own)) { @@ -365,6 +406,7 @@ MOZ_ALWAYS_INLINE bool Proxy::setInternal(JSContext* cx, HandleObject proxy, if (!CheckRecursionLimit(cx)) { return false; } + const BaseProxyHandler* handler = proxy->as().handler(); AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true); if (!policy.allowed()) { @@ -374,6 +416,17 @@ MOZ_ALWAYS_INLINE bool Proxy::setInternal(JSContext* cx, HandleObject proxy, return result.succeed(); } + // Private field accesses have different semantics depending on the kind + // of proxy involved, and so take a different path compared to regular + // [[Set]] operations. + // + // This doesn't interact with hasPrototype, as PrivateFields are always + // own propertiers, and so we never deal with prototype traversals. + bool isPrivate = JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id)->isPrivateName(); + if (isPrivate) { + return handler->setPrivate(cx, proxy, id, v, receiver, result); + } + // Special case. See the comment on BaseProxyHandler::mHasPrototype. if (handler->hasPrototype()) { return handler->BaseProxyHandler::set(cx, proxy, id, v, receiver, result); @@ -664,6 +717,7 @@ void ProxyObject::trace(JSTracer* trc, JSObject* obj) { ProxyObject* proxy = &obj->as(); TraceEdge(trc, proxy->shapePtr(), "ProxyObject_shape"); + TraceNullableEdge(trc, proxy->slotOfExpando(), "expando"); #ifdef DEBUG if (TlsContext.get()->isStrictProxyCheckingEnabled() && diff --git a/js/src/tests/non262/PrivateName/proxy-1.js b/js/src/tests/non262/PrivateName/proxy-1.js new file mode 100644 index 000000000000..371885d9b2f6 --- /dev/null +++ b/js/src/tests/non262/PrivateName/proxy-1.js @@ -0,0 +1,21 @@ +// |reftest| shell-option(--enable-private-fields) + +class A { + #x = 10; + g() { + return this.#x; + } +}; + +var p = new Proxy(new A, {}); +var completed = false; +try { + p.g(); + completed = true; +} catch (e) { + assertEq(e instanceof TypeError, true); +} +assertEq(completed, false); + + +if (typeof reportCompare === 'function') reportCompare(0, 0); diff --git a/js/src/tests/non262/PrivateName/proxy-init-set.js b/js/src/tests/non262/PrivateName/proxy-init-set.js new file mode 100644 index 000000000000..4d3e881c3ea1 --- /dev/null +++ b/js/src/tests/non262/PrivateName/proxy-init-set.js @@ -0,0 +1,75 @@ +// |reftest| shell-option(--enable-private-fields) + +// Ensure that the distinction between Proxy Init and Proxy Set holds + +function assertThrowsTypeError(f) { + var type; + try { + f(); + } catch (ex) { + type = ex.name; + } + assertEq(type, 'TypeError'); +} + + + +var target = {}; +var p1 = new Proxy(target, {}); +var p2 = new Proxy(target, {}); + +class Base { + constructor(o) { + return o; + } +} + +class A extends Base { + #field = 10; + static gf(o) { + return o.#field; + } + static sf(o) { + o.#field = 15; + } +} + +class B extends Base { + #field = 25; + static gf(o) { + return o.#field; + } + static sf(o) { + o.#field = 20; + } +} + +// Verify field handling on the proxy we install it on. +new A(p1); +assertEq(A.gf(p1), 10); +A.sf(p1) +assertEq(A.gf(p1), 15); + +// Despite P1 being stamped with A's field, it shouldn't +// be sufficient to set B's field. +assertThrowsTypeError(() => B.sf(p1)); +assertThrowsTypeError(() => B.gf(p1)); +assertThrowsTypeError(() => B.sf(p1)); +new B(p1); +assertEq(B.gf(p1), 25); +B.sf(p1); +assertEq(B.gf(p1), 20); + +// A's field should't be on the target +assertThrowsTypeError(() => A.gf(target)); + +// Can't set the field, doesn't exist +assertThrowsTypeError(() => A.sf(p2)); + +// Definitely can't get the field, doesn't exist. +assertThrowsTypeError(() => A.gf(p2)); + +// Still should't be on the target. +assertThrowsTypeError(() => A.gf(target)); + +if (typeof reportCompare === 'function') reportCompare(0, 0); \ No newline at end of file diff --git a/js/src/vm/ProxyObject.cpp b/js/src/vm/ProxyObject.cpp index 2b29dcdf8f8d..048041e67e81 100644 --- a/js/src/vm/ProxyObject.cpp +++ b/js/src/vm/ProxyObject.cpp @@ -61,6 +61,10 @@ void ProxyObject::init(const BaseProxyHandler* handler, HandleValue priv, } else { setSameCompartmentPrivate(priv); } + + // The expando slot is nullptr until required by the installation of + // a private field. + setExpando(nullptr); } /* static */ @@ -267,6 +271,18 @@ inline void ProxyObject::setPrivate(const Value& priv) { *slotOfPrivate() = priv; } +void ProxyObject::setExpando(JSObject* expando) { + // Ensure that we don't accidentally end up pointing to a + // grey object, which would violate GC invariants. + MOZ_ASSERT_IF(IsMarkedBlack(this) && expando, + !JS::GCThingIsMarkedGray(JS::GCCellPtr(expando))); + + // Ensure we're in the same compartment as the proxy object: Don't want the + // expando to end up as a CCW. + MOZ_ASSERT_IF(expando, expando->compartment() == compartment()); + *slotOfExpando() = ObjectOrNullValue(expando); +} + void ProxyObject::nuke() { // Notify the zone that a delegate is no longer a delegate. Be careful not to // expose this pointer, because it has already been removed from the wrapper diff --git a/js/src/vm/ProxyObject.h b/js/src/vm/ProxyObject.h index 7c84e992fc57..e2155fe7a78c 100644 --- a/js/src/vm/ProxyObject.h +++ b/js/src/vm/ProxyObject.h @@ -68,6 +68,9 @@ class ProxyObject : public JSObject { HandleValueVector values); const Value& private_() const { return GetProxyPrivate(this); } + const Value& expando() const { return GetProxyExpando(this); } + + void setExpando(JSObject* expando); void setCrossCompartmentPrivate(const Value& priv); void setSameCompartmentPrivate(const Value& priv); @@ -109,6 +112,11 @@ class ProxyObject : public JSObject { &detail::GetProxyDataLayout(this)->values()->privateSlot); } + GCPtrValue* slotOfExpando() { + return reinterpret_cast( + &detail::GetProxyDataLayout(this)->values()->expandoSlot); + } + void setPrivate(const Value& priv); static bool isValidProxyClass(const JSClass* clasp) {