[JSC] Private symbols should not be trapped by proxy handler
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 03:48:36 +0000 (03:48 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 03:48:36 +0000 (03:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154817

Reviewed by Mark Lam.

Since the runtime has some assumptions on the properties associated with the private symbols, ES6 Proxy should not trap these property operations.
For example, in ArrayIteratorPrototype.js

    var itemKind = this.@arrayIterationKind;
    if (itemKind === @undefined)
        throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");

Here, we assume that only the array iterator has the @arrayIterationKind property that value is non-undefined.
But If we implement Proxy with the get handler, that returns a non-undefined value for every operations, we accidentally assumes that the given value is an array iterator.

To avoid these situation, we perform the default operations onto property operations with private symbols.

* runtime/ProxyObject.cpp:
(JSC::performProxyGet):
(JSC::ProxyObject::performInternalMethodGetOwnProperty):
(JSC::ProxyObject::performHasProperty):
(JSC::ProxyObject::performPut):
(JSC::ProxyObject::performDelete):
(JSC::ProxyObject::deleteProperty):
(JSC::ProxyObject::deletePropertyByIndex):
* tests/stress/proxy-basic.js:
* tests/stress/proxy-with-private-symbols.js: Added.
(assert):
(let.handler.getOwnPropertyDescriptor):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197383 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/ProxyObject.cpp
Source/JavaScriptCore/tests/stress/proxy-basic.js
Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js [new file with mode: 0644]

index 02b87e0..4c46a82 100644 (file)
@@ -1,3 +1,35 @@
+2016-02-29  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        [JSC] Private symbols should not be trapped by proxy handler
+        https://bugs.webkit.org/show_bug.cgi?id=154817
+
+        Reviewed by Mark Lam.
+
+        Since the runtime has some assumptions on the properties associated with the private symbols, ES6 Proxy should not trap these property operations.
+        For example, in ArrayIteratorPrototype.js
+
+            var itemKind = this.@arrayIterationKind;
+            if (itemKind === @undefined)
+                throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");
+
+        Here, we assume that only the array iterator has the @arrayIterationKind property that value is non-undefined.
+        But If we implement Proxy with the get handler, that returns a non-undefined value for every operations, we accidentally assumes that the given value is an array iterator.
+
+        To avoid these situation, we perform the default operations onto property operations with private symbols.
+
+        * runtime/ProxyObject.cpp:
+        (JSC::performProxyGet):
+        (JSC::ProxyObject::performInternalMethodGetOwnProperty):
+        (JSC::ProxyObject::performHasProperty):
+        (JSC::ProxyObject::performPut):
+        (JSC::ProxyObject::performDelete):
+        (JSC::ProxyObject::deleteProperty):
+        (JSC::ProxyObject::deletePropertyByIndex):
+        * tests/stress/proxy-basic.js:
+        * tests/stress/proxy-with-private-symbols.js: Added.
+        (assert):
+        (let.handler.getOwnPropertyDescriptor):
+
 2016-02-29  Filip Pizlo  <fpizlo@apple.com>
 
         regress/script-tests/double-pollution-putbyoffset.js.ftl-eager timed out because of a lock ordering deadlock involving InferredType and CodeBlock
index 3ed1539..2f8e577 100644 (file)
@@ -95,6 +95,15 @@ static EncodedJSValue performProxyGet(ExecState* exec, EncodedJSValue thisValue,
     }
 
     ProxyObject* proxyObject = jsCast<ProxyObject*>(proxyObjectAsObject);
+    JSObject* target = proxyObject->target();
+
+    auto performDefaultGet = [&] {
+        return JSValue::encode(target->get(exec, propertyName));
+    };
+
+    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid())))
+        return performDefaultGet();
+
     JSValue handlerValue = proxyObject->handler();
     if (handlerValue.isNull())
         return throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
@@ -106,9 +115,8 @@ static EncodedJSValue performProxyGet(ExecState* exec, EncodedJSValue thisValue,
     if (exec->hadException())
         return JSValue::encode(jsUndefined());
 
-    JSObject* target = proxyObject->target();
     if (getHandler.isUndefined())
-        return JSValue::encode(target->get(exec, propertyName));
+        return performDefaultGet();
 
     MarkedArgumentBuffer arguments;
     arguments.append(target);
@@ -138,6 +146,15 @@ static EncodedJSValue performProxyGet(ExecState* exec, EncodedJSValue thisValue,
 bool ProxyObject::performInternalMethodGetOwnProperty(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
 {
     VM& vm = exec->vm();
+    JSObject* target = this->target();
+
+    auto performDefaultGetOwnProperty = [&] {
+        return target->methodTable(vm)->getOwnPropertySlot(target, exec, propertyName, slot);
+    };
+
+    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid())))
+        return performDefaultGetOwnProperty();
+
     JSValue handlerValue = this->handler();
     if (handlerValue.isNull()) {
         throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
@@ -150,9 +167,8 @@ bool ProxyObject::performInternalMethodGetOwnProperty(ExecState* exec, PropertyN
     JSValue getOwnPropertyDescriptorMethod = handler->getMethod(exec, callData, callType, makeIdentifier(vm, "getOwnPropertyDescriptor"), ASCIILiteral("'getOwnPropertyDescriptor' property of a Proxy's handler should be callable."));
     if (exec->hadException())
         return false;
-    JSObject* target = this->target();
     if (getOwnPropertyDescriptorMethod.isUndefined())
-        return target->methodTable(vm)->getOwnPropertySlot(target, exec, propertyName, slot);
+        return performDefaultGetOwnProperty();
 
     MarkedArgumentBuffer arguments;
     arguments.append(target);
@@ -226,8 +242,16 @@ bool ProxyObject::performInternalMethodGetOwnProperty(ExecState* exec, PropertyN
 bool ProxyObject::performHasProperty(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
 {
     VM& vm = exec->vm();
+    JSObject* target = this->target();
     slot.setValue(this, None, jsUndefined()); // Nobody should rely on our value, but be safe and protect against any bad actors reading our value.
-    
+
+    auto performDefaultHasProperty = [&] {
+        return target->methodTable(vm)->getOwnPropertySlot(target, exec, propertyName, slot);
+    };
+
+    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid())))
+        return performDefaultHasProperty();
+
     JSValue handlerValue = this->handler();
     if (handlerValue.isNull()) {
         throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
@@ -240,9 +264,8 @@ bool ProxyObject::performHasProperty(ExecState* exec, PropertyName propertyName,
     JSValue hasMethod = handler->getMethod(exec, callData, callType, vm.propertyNames->has, ASCIILiteral("'has' property of a Proxy's handler should be callable."));
     if (exec->hadException())
         return false;
-    JSObject* target = this->target();
     if (hasMethod.isUndefined())
-        return target->methodTable(vm)->getOwnPropertySlot(target, exec, propertyName, slot);
+        return performDefaultHasProperty();
 
     MarkedArgumentBuffer arguments;
     arguments.append(target);
@@ -310,9 +333,13 @@ bool ProxyObject::getOwnPropertySlotByIndex(JSObject* object, ExecState* exec, u
 }
 
 template <typename PerformDefaultPutFunction>
-void ProxyObject::performPut(ExecState* exec, JSValue putValue, JSValue thisValue, PropertyName propertyName, PerformDefaultPutFunction performDefaultPutFunction)
+void ProxyObject::performPut(ExecState* exec, JSValue putValue, JSValue thisValue, PropertyName propertyName, PerformDefaultPutFunction performDefaultPut)
 {
     VM& vm = exec->vm();
+
+    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid())))
+        return performDefaultPut();
+
     JSValue handlerValue = this->handler();
     if (handlerValue.isNull()) {
         throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
@@ -327,7 +354,7 @@ void ProxyObject::performPut(ExecState* exec, JSValue putValue, JSValue thisValu
         return;
     JSObject* target = this->target();
     if (setMethod.isUndefined()) {
-        performDefaultPutFunction();
+        performDefaultPut();
         return;
     }
 
@@ -487,9 +514,13 @@ ConstructType ProxyObject::getConstructData(JSCell* cell, ConstructData& constru
 }
 
 template <typename DefaultDeleteFunction>
-bool ProxyObject::performDelete(ExecState* exec, PropertyName propertyName, DefaultDeleteFunction defaultDeleteFunction)
+bool ProxyObject::performDelete(ExecState* exec, PropertyName propertyName, DefaultDeleteFunction performDefaultDelete)
 {
     VM& vm = exec->vm();
+
+    if (vm.propertyNames->isPrivateName(Identifier::fromUid(&vm, propertyName.uid())))
+        return performDefaultDelete();
+
     JSValue handlerValue = this->handler();
     if (handlerValue.isNull()) {
         throwVMTypeError(exec, ASCIILiteral("Proxy 'handler' is null. It should be an Object."));
@@ -504,7 +535,7 @@ bool ProxyObject::performDelete(ExecState* exec, PropertyName propertyName, Defa
         return false;
     JSObject* target = this->target();
     if (deletePropertyMethod.isUndefined())
-        return defaultDeleteFunction();
+        return performDefaultDelete();
 
     MarkedArgumentBuffer arguments;
     arguments.append(target);
@@ -537,11 +568,11 @@ bool ProxyObject::performDelete(ExecState* exec, PropertyName propertyName, Defa
 bool ProxyObject::deleteProperty(JSCell* cell, ExecState* exec, PropertyName propertyName)
 {
     ProxyObject* thisObject = jsCast<ProxyObject*>(cell);
-    auto defaultDelete = [&] () -> bool {
+    auto performDefaultDelete = [&] () -> bool {
         JSObject* target = jsCast<JSObject*>(thisObject->target());
         return target->methodTable(exec->vm())->deleteProperty(target, exec, propertyName);
     };
-    return thisObject->performDelete(exec, propertyName, defaultDelete);
+    return thisObject->performDelete(exec, propertyName, performDefaultDelete);
 }
 
 bool ProxyObject::deletePropertyByIndex(JSCell* cell, ExecState* exec, unsigned propertyName)
@@ -550,11 +581,11 @@ bool ProxyObject::deletePropertyByIndex(JSCell* cell, ExecState* exec, unsigned
     Identifier ident = Identifier::from(exec, propertyName); 
     if (exec->hadException())
         return false;
-    auto defaultDelete = [&] () -> bool {
+    auto performDefaultDelete = [&] () -> bool {
         JSObject* target = jsCast<JSObject*>(thisObject->target());
         return target->methodTable(exec->vm())->deletePropertyByIndex(target, exec, propertyName);
     };
-    return thisObject->performDelete(exec, ident.impl(), defaultDelete);
+    return thisObject->performDelete(exec, ident.impl(), performDefaultDelete);
 }
 
 void ProxyObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
index 521470b..04162d0 100644 (file)
@@ -281,32 +281,6 @@ assert(Proxy.prototype === undefined);
 }
 
 {
-    let theTarget = [];
-    let sawPrivateSymbolAsString = false;
-    let handler = {
-        get: function(target, propName, proxyArg) {
-            if (typeof propName === "string")
-                sawPrivateSymbolAsString = propName === "PrivateSymbol.arrayIterationKind";
-            return target[propName];
-        }
-    };
-
-    let proxy = new Proxy(theTarget, handler);
-    for (let i = 0; i < 100; i++) {
-        let threw = false;
-        try {
-            proxy[Symbol.iterator]().next.call(proxy);
-        } catch(e) {
-            // this will throw because we conver private symbols to strings.
-            threw = true;
-        }
-        assert(threw);
-        assert(sawPrivateSymbolAsString);
-        sawPrivateSymbolAsString = false;
-    }
-}
-
-{
     let prop = Symbol();
     let theTarget = { };
     Object.defineProperty(theTarget, prop, {
diff --git a/Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js b/Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js
new file mode 100644 (file)
index 0000000..7cee499
--- /dev/null
@@ -0,0 +1,88 @@
+function assert(b) {
+    if (!b)
+        throw new Error("bad assertion.");
+}
+
+// Currently, only "get", "getOwnPropertyDescriptor", and "set" are testable.
+
+{
+    let theTarget = [];
+    let sawPrivateSymbolAsString = false;
+    let handler = {
+        get: function(target, propName, proxyArg) {
+            if (typeof propName === "string")
+                sawPrivateSymbolAsString = propName === "PrivateSymbol.arrayIterationKind";
+            return target[propName];
+        }
+    };
+
+    let proxy = new Proxy(theTarget, handler);
+    for (let i = 0; i < 100; i++) {
+        let threw = false;
+        try {
+            proxy[Symbol.iterator]().next.call(proxy);
+        } catch(e) {
+            // this will throw because we convert private symbols to strings.
+            assert(e.message === "%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");
+            threw = true;
+        }
+        assert(threw);
+        assert(!sawPrivateSymbolAsString);
+        sawPrivateSymbolAsString = false;
+    }
+}
+
+{
+    let theTarget = [];
+    let sawPrivateSymbolAsString = false;
+    let handler = {
+        getOwnPropertyDescriptor: function(theTarget, propName) {
+            if (typeof propName === "string")
+                sawPrivateSymbolAsString = propName === "PrivateSymbol.arrayIterationKind";
+            return target[propName];
+        }
+    };
+
+    let proxy = new Proxy(theTarget, handler);
+    for (let i = 0; i < 100; i++) {
+        let threw = false;
+        try {
+            proxy[Symbol.iterator]().next.call(proxy);
+        } catch(e) {
+            // this will throw because we convert private symbols to strings.
+            assert(e.message === "%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");
+            threw = true;
+        }
+        assert(threw);
+        assert(!sawPrivateSymbolAsString);
+        sawPrivateSymbolAsString = false;
+    }
+}
+
+{
+    let theTarget = [1,2,3,4,5];
+    let iterator = theTarget[Symbol.iterator]();
+    let sawPrivateSymbolAsString = false;
+    let handler = {
+        set: function(theTarget, propName, value, receiver) {
+            if (typeof propName === "string")
+                sawPrivateSymbolAsString = propName === "PrivateSymbol.arrayIterationKind";
+            return target[propName];
+        }
+    };
+
+    let proxy = new Proxy(iterator, handler);
+    for (let i = 0; i < 100; i++) {
+        let threw = false;
+        try {
+            proxy.next();
+        } catch(e) {
+            // this will throw because we convert private symbols to strings.
+            assert(e.message === "%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");
+            threw = true;
+        }
+        assert(!threw);
+        assert(!sawPrivateSymbolAsString);
+        sawPrivateSymbolAsString = false;
+    }
+}