Insert exception check around toPropertyKey call
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Mar 2015 11:08:49 +0000 (11:08 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Mar 2015 11:08:49 +0000 (11:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=142922

Reviewed by Geoffrey Garen.

In some places, exception check is missing after/before toPropertyKey.
However, since it calls toString, it's observable to users,

Missing exception checks in Object.prototype methods can be
observed since it would be overridden with toObject(null/undefined) errors.
We inserted exception checks after toPropertyKey.

Missing exception checks in GetById related code can be
observed since it would be overridden with toObject(null/undefined) errors.
In this case, we need to insert exception checks before/after toPropertyKey
since RequireObjectCoercible followed by toPropertyKey can cause exceptions.

JSValue::get checks null/undefined and raise an exception if |this| is null or undefined.
However, we need to check whether the baseValue is object coercible before executing JSValue::toPropertyKey.
According to the spec, we first perform RequireObjectCoercible and check the exception.
And second, we perform ToPropertyKey and check the exception.
Since JSValue::toPropertyKey can cause toString call, this is observable to users.
For example, if the target is not object coercible,
ToPropertyKey should not be executed, and toString should not be executed by ToPropertyKey.
So the order of observable actions (RequireObjectCoercible and ToPropertyKey) should be correct to the spec.

This patch introduces JSValue::requireObjectCoercible and use it because of the following 2 reasons.

1. Using toObject instead of requireObjectCoercible produces unnecessary wrapper object.

toObject converts primitive types into wrapper objects.
But it is not efficient since wrapper objects are not necessary
if we look up methods from primitive values's prototype. (using synthesizePrototype is better).

2. Using the result of toObject is not correct to the spec.

To align to the spec correctly, we cannot use JSObject::get
by using the wrapper object produced by the toObject suggested in (1).
If we use JSObject that is converted by toObject, getter will be called by using this JSObject as |this|.
It is not correct since getter should be called with the original |this| value that may be primitive types.

So in this patch, we use JSValue::requireObjectCoercible
to check the target is object coercible and raise an error if it's not.

* dfg/DFGOperations.cpp:
* jit/JITOperations.cpp:
(JSC::getByVal):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::getByVal):
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
* runtime/JSCJSValue.h:
* runtime/JSCJSValueInlines.h:
(JSC::JSValue::requireObjectCoercible):
* runtime/ObjectPrototype.cpp:
(JSC::objectProtoFuncHasOwnProperty):
(JSC::objectProtoFuncDefineGetter):
(JSC::objectProtoFuncDefineSetter):
(JSC::objectProtoFuncLookupGetter):
(JSC::objectProtoFuncLookupSetter):
(JSC::objectProtoFuncPropertyIsEnumerable):
* tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js: Added.
(shouldThrow):
(if):
* tests/stress/exception-in-to-property-key-should-be-handled-early.js: Added.
(shouldThrow):
(.):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
Source/JavaScriptCore/runtime/JSCJSValue.h
Source/JavaScriptCore/runtime/JSCJSValueInlines.h
Source/JavaScriptCore/runtime/ObjectPrototype.cpp
Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early.js [new file with mode: 0644]

index 274f28d..e0ed8bb 100644 (file)
@@ -1,3 +1,73 @@
+2015-03-27  Yusuke Suzuki  <utatane.tea@gmail.com>
+
+        Insert exception check around toPropertyKey call
+        https://bugs.webkit.org/show_bug.cgi?id=142922
+
+        Reviewed by Geoffrey Garen.
+
+        In some places, exception check is missing after/before toPropertyKey.
+        However, since it calls toString, it's observable to users,
+
+        Missing exception checks in Object.prototype methods can be
+        observed since it would be overridden with toObject(null/undefined) errors.
+        We inserted exception checks after toPropertyKey.
+
+        Missing exception checks in GetById related code can be
+        observed since it would be overridden with toObject(null/undefined) errors.
+        In this case, we need to insert exception checks before/after toPropertyKey
+        since RequireObjectCoercible followed by toPropertyKey can cause exceptions.
+
+        JSValue::get checks null/undefined and raise an exception if |this| is null or undefined.
+        However, we need to check whether the baseValue is object coercible before executing JSValue::toPropertyKey.
+        According to the spec, we first perform RequireObjectCoercible and check the exception.
+        And second, we perform ToPropertyKey and check the exception.
+        Since JSValue::toPropertyKey can cause toString call, this is observable to users.
+        For example, if the target is not object coercible,
+        ToPropertyKey should not be executed, and toString should not be executed by ToPropertyKey.
+        So the order of observable actions (RequireObjectCoercible and ToPropertyKey) should be correct to the spec.
+
+        This patch introduces JSValue::requireObjectCoercible and use it because of the following 2 reasons.
+
+        1. Using toObject instead of requireObjectCoercible produces unnecessary wrapper object.
+
+        toObject converts primitive types into wrapper objects.
+        But it is not efficient since wrapper objects are not necessary
+        if we look up methods from primitive values's prototype. (using synthesizePrototype is better).
+
+        2. Using the result of toObject is not correct to the spec.
+
+        To align to the spec correctly, we cannot use JSObject::get
+        by using the wrapper object produced by the toObject suggested in (1).
+        If we use JSObject that is converted by toObject, getter will be called by using this JSObject as |this|.
+        It is not correct since getter should be called with the original |this| value that may be primitive types.
+
+        So in this patch, we use JSValue::requireObjectCoercible
+        to check the target is object coercible and raise an error if it's not.
+
+        * dfg/DFGOperations.cpp:
+        * jit/JITOperations.cpp:
+        (JSC::getByVal):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::getByVal):
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::SLOW_PATH_DECL):
+        * runtime/JSCJSValue.h:
+        * runtime/JSCJSValueInlines.h:
+        (JSC::JSValue::requireObjectCoercible):
+        * runtime/ObjectPrototype.cpp:
+        (JSC::objectProtoFuncHasOwnProperty):
+        (JSC::objectProtoFuncDefineGetter):
+        (JSC::objectProtoFuncDefineSetter):
+        (JSC::objectProtoFuncLookupGetter):
+        (JSC::objectProtoFuncLookupSetter):
+        (JSC::objectProtoFuncPropertyIsEnumerable):
+        * tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js: Added.
+        (shouldThrow):
+        (if):
+        * tests/stress/exception-in-to-property-key-should-be-handled-early.js: Added.
+        (shouldThrow):
+        (.):
+
 2015-03-26  Joseph Pecoraro  <pecoraro@apple.com>
 
         WebContent Crash when instantiating class with Type Profiling enabled
index 62eca89..7614e2b 100644 (file)
@@ -298,7 +298,12 @@ EncodedJSValue JIT_OPERATION operationGetByVal(ExecState* exec, EncodedJSValue e
         }
     }
 
+    baseValue.requireObjectCoercible(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     auto propertyName = property.toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     return JSValue::encode(baseValue.get(exec, propertyName));
 }
 
@@ -327,6 +332,8 @@ EncodedJSValue JIT_OPERATION operationGetByValCell(ExecState* exec, JSCell* base
     }
 
     auto propertyName = property.toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     return JSValue::encode(JSValue(base).get(exec, propertyName));
 }
 
index c651eac..6451650 100644 (file)
@@ -1386,7 +1386,12 @@ static JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript, R
         return baseValue.get(exec, i);
     }
 
+    baseValue.requireObjectCoercible(exec);
+    if (exec->hadException())
+        return jsUndefined();
     auto property = subscript.toPropertyKey(exec);
+    if (exec->hadException())
+        return jsUndefined();
     return baseValue.get(exec, property);
 }
 
@@ -1522,7 +1527,12 @@ EncodedJSValue JIT_OPERATION operationGetByValString(ExecState* exec, EncodedJSV
                 ctiPatchCallByReturnAddress(exec->codeBlock(), ReturnAddressPtr(OUR_RETURN_ADDRESS), FunctionPtr(operationGetByValDefault));
         }
     } else {
+        baseValue.requireObjectCoercible(exec);
+        if (exec->hadException())
+            return JSValue::encode(jsUndefined());
         auto property = subscript.toPropertyKey(exec);
+        if (exec->hadException())
+            return JSValue::encode(jsUndefined());
         result = baseValue.get(exec, property);
     }
 
index db4310e..279285a 100644 (file)
@@ -743,7 +743,12 @@ inline JSValue getByVal(ExecState* exec, JSValue baseValue, JSValue subscript)
         return baseValue.get(exec, i);
     }
 
+    baseValue.requireObjectCoercible(exec);
+    if (exec->hadException())
+        return jsUndefined();
     auto property = subscript.toPropertyKey(exec);
+    if (exec->hadException())
+        return jsUndefined();
     return baseValue.get(exec, property);
 }
 
index a3addd2..653ebdb 100644 (file)
@@ -582,7 +582,7 @@ SLOW_PATH_DECL(slow_path_get_direct_pname)
     JSValue baseValue = OP_C(2).jsValue();
     JSValue property = OP(3).jsValue();
     ASSERT(property.isString());
-    RETURN(baseValue.get(exec, property.toPropertyKey(exec)));
+    RETURN(baseValue.get(exec, asString(property)->toIdentifier(exec)));
 }
 
 SLOW_PATH_DECL(slow_path_get_property_enumerator)
index 360e46e..21fdf33 100644 (file)
@@ -302,6 +302,7 @@ public:
     void dumpForBacktrace(PrintStream&) const;
 
     JS_EXPORT_PRIVATE JSObject* synthesizePrototype(ExecState*) const;
+    bool requireObjectCoercible(ExecState*) const;
 
     // Constants used for Int52. Int52 isn't part of JSValue right now, but JSValues may be
     // converted to Int52s and back again.
index 463f9fa..a40f98f 100644 (file)
@@ -26,6 +26,7 @@
 #ifndef JSValueInlines_h
 #define JSValueInlines_h
 
+#include "ExceptionHelpers.h"
 #include "Identifier.h"
 #include "InternalFunction.h"
 #include "JSCJSValue.h"
@@ -916,6 +917,14 @@ inline TriState JSValue::pureToBoolean() const
     return isTrue() ? TrueTriState : FalseTriState;
 }
 
+ALWAYS_INLINE bool JSValue::requireObjectCoercible(ExecState* exec) const
+{
+    if (!isUndefinedOrNull())
+        return true;
+    exec->vm().throwException(exec, createNotAnObjectError(exec, *this));
+    return false;
+}
+
 } // namespace JSC
 
 #endif // JSValueInlines_h
index 00d56c6..ef854b0 100644 (file)
@@ -85,7 +85,10 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncValueOf(ExecState* exec)
 EncodedJSValue JSC_HOST_CALL objectProtoFuncHasOwnProperty(ExecState* exec)
 {
     JSValue thisValue = exec->thisValue().toThis(exec, StrictMode);
-    return JSValue::encode(jsBoolean(thisValue.toObject(exec)->hasOwnProperty(exec, exec->argument(0).toPropertyKey(exec))));
+    auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+    return JSValue::encode(jsBoolean(thisValue.toObject(exec)->hasOwnProperty(exec, propertyName)));
 }
 
 EncodedJSValue JSC_HOST_CALL objectProtoFuncIsPrototypeOf(ExecState* exec)
@@ -118,13 +121,17 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncDefineGetter(ExecState* exec)
     if (getCallData(get, callData) == CallTypeNone)
         return throwVMError(exec, createTypeError(exec, ASCIILiteral("invalid getter usage")));
 
+    auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+
     PropertyDescriptor descriptor;
     descriptor.setGetter(get);
     descriptor.setEnumerable(true);
     descriptor.setConfigurable(true);
 
     bool shouldThrow = true;
-    thisObject->methodTable(exec->vm())->defineOwnProperty(thisObject, exec, exec->argument(0).toPropertyKey(exec), descriptor, shouldThrow);
+    thisObject->methodTable(exec->vm())->defineOwnProperty(thisObject, exec, propertyName, descriptor, shouldThrow);
 
     return JSValue::encode(jsUndefined());
 }
@@ -140,13 +147,17 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncDefineSetter(ExecState* exec)
     if (getCallData(set, callData) == CallTypeNone)
         return throwVMError(exec, createTypeError(exec, ASCIILiteral("invalid setter usage")));
 
+    auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+
     PropertyDescriptor descriptor;
     descriptor.setSetter(set);
     descriptor.setEnumerable(true);
     descriptor.setConfigurable(true);
 
     bool shouldThrow = true;
-    thisObject->methodTable(exec->vm())->defineOwnProperty(thisObject, exec, exec->argument(0).toPropertyKey(exec), descriptor, shouldThrow);
+    thisObject->methodTable(exec->vm())->defineOwnProperty(thisObject, exec, propertyName, descriptor, shouldThrow);
 
     return JSValue::encode(jsUndefined());
 }
@@ -157,9 +168,12 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncLookupGetter(ExecState* exec)
     if (exec->hadException())
         return JSValue::encode(jsUndefined());
 
+    auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+
     PropertySlot slot(thisObject);
-    if (thisObject->getPropertySlot(exec, exec->argument(0).toPropertyKey(exec), slot)
-        && slot.isAccessor()) {
+    if (thisObject->getPropertySlot(exec, propertyName, slot) && slot.isAccessor()) {
         GetterSetter* getterSetter = slot.getterSetter();
         return getterSetter->isGetterNull() ? JSValue::encode(jsUndefined()) : JSValue::encode(getterSetter->getter());
     }
@@ -173,9 +187,12 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncLookupSetter(ExecState* exec)
     if (exec->hadException())
         return JSValue::encode(jsUndefined());
 
+    auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
+
     PropertySlot slot(thisObject);
-    if (thisObject->getPropertySlot(exec, exec->argument(0).toPropertyKey(exec), slot)
-        && slot.isAccessor()) {
+    if (thisObject->getPropertySlot(exec, propertyName, slot) && slot.isAccessor()) {
         GetterSetter* getterSetter = slot.getterSetter();
         return getterSetter->isSetterNull() ? JSValue::encode(jsUndefined()) : JSValue::encode(getterSetter->setter());
     }
@@ -185,9 +202,13 @@ EncodedJSValue JSC_HOST_CALL objectProtoFuncLookupSetter(ExecState* exec)
 
 EncodedJSValue JSC_HOST_CALL objectProtoFuncPropertyIsEnumerable(ExecState* exec)
 {
-    JSObject* thisObject = exec->thisValue().toThis(exec, StrictMode).toObject(exec);
     auto propertyName = exec->argument(0).toPropertyKey(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
 
+    JSObject* thisObject = exec->thisValue().toThis(exec, StrictMode).toObject(exec);
+    if (exec->hadException())
+        return JSValue::encode(jsUndefined());
     PropertyDescriptor descriptor;
     bool enumerable = thisObject->getOwnPropertyDescriptor(exec, propertyName, descriptor) && descriptor.enumerable();
     return JSValue::encode(jsBoolean(enumerable));
diff --git a/Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js b/Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js
new file mode 100644 (file)
index 0000000..f7cc01d
--- /dev/null
@@ -0,0 +1,73 @@
+
+var propertyKey = {
+    toString() {
+        throw new Error("propertyKey.toString is called.");
+    }
+};
+
+function shouldThrow(func, message) {
+    var error = null;
+    try {
+        func();
+    } catch (e) {
+        error = e;
+    }
+    if (!error)
+        throw new Error("not thrown.");
+    if (String(error) !== message)
+        throw new Error("bad error: " + String(error));
+}
+
+var object = {};
+
+shouldThrow(function () {
+    object.hasOwnProperty(propertyKey);
+}, "Error: propertyKey.toString is called.");
+
+shouldThrow(function () {
+    Object.prototype.hasOwnProperty.call(null, propertyKey);
+}, "Error: propertyKey.toString is called.");
+
+shouldThrow(function () {
+    Object.prototype.hasOwnProperty.call(null, 'ok');
+}, "TypeError: null is not an object (evaluating 'Object.prototype.hasOwnProperty.call(null, 'ok')')");
+
+shouldThrow(function () {
+    object.propertyIsEnumerable(propertyKey);
+}, "Error: propertyKey.toString is called.");
+
+// ToPropertyKey is first, ToObject is following.
+shouldThrow(function () {
+    Object.prototype.propertyIsEnumerable.call(null, propertyKey);
+}, "Error: propertyKey.toString is called.");
+
+shouldThrow(function () {
+    // ToPropertyKey is first, ToObject is following.
+    Object.prototype.propertyIsEnumerable.call(null, 'ok');
+}, "TypeError: null is not an object (evaluating 'Object.prototype.propertyIsEnumerable.call(null, 'ok')')");
+
+shouldThrow(function () {
+    object.__defineGetter__(propertyKey, function () {
+        return 'NG';
+    });
+}, "Error: propertyKey.toString is called.");
+
+if (Object.getOwnPropertyDescriptor(object, ''))
+    throw new Error("bad descriptor");
+
+shouldThrow(function () {
+    object.__defineSetter__(propertyKey, function () {
+        return 'NG';
+    });
+}, "Error: propertyKey.toString is called.");
+
+if (Object.getOwnPropertyDescriptor(object, ''))
+    throw new Error("bad descriptor");
+
+shouldThrow(function () {
+    object.__lookupGetter__(propertyKey);
+}, "Error: propertyKey.toString is called.");
+
+shouldThrow(function () {
+    object.__lookupSetter__(propertyKey);
+}, "Error: propertyKey.toString is called.");
diff --git a/Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early.js b/Source/JavaScriptCore/tests/stress/exception-in-to-property-key-should-be-handled-early.js
new file mode 100644 (file)
index 0000000..e511d03
--- /dev/null
@@ -0,0 +1,139 @@
+
+var propertyKey = {
+    toString() {
+        throw new Error("propertyKey.toString is called.");
+    }
+};
+
+function shouldThrow(func, message) {
+    var error = null;
+    try {
+        func();
+    } catch (e) {
+        error = e;
+    }
+    if (!error)
+        throw new Error("not thrown.");
+    if (String(error) !== message)
+        throw new Error("bad error: " + String(error));
+}
+
+// GetByVal.
+(function () {
+    var called = null;
+    var toStringCalled = false;
+    var property = {
+        toString() {
+            toStringCalled = true;
+            return "value";
+        }
+    };
+    var object = {
+        get value() {
+        },
+        set value(val) {
+        }
+    };
+    Object.defineProperty(object, '', {
+        get: function () {
+            called = "getter for '' is called.";
+        }
+    });
+
+    function test(object, property) {
+        object[property];
+    }
+    noInline(test);
+
+    shouldThrow(function () { test(object, propertyKey); }, "Error: propertyKey.toString is called.");
+    if (called)
+        throw new Error(called);
+    toStringCalled = false;
+    shouldThrow(function () { test(null, propertyKey); }, "TypeError: null is not an object (evaluating 'object[property]')");
+    if (toStringCalled)
+        throw new Error("toString is called.");
+}());
+
+// GetByVal DFG.
+(function () {
+    var called = null;
+    var toStringCalled = false;
+    var property = {
+        toString() {
+            toStringCalled = true;
+            return "value";
+        }
+    };
+    var object = {
+        get ""() {
+            called = "getter for '' is called.";
+        },
+        set ""(val) {
+            called = "setter for '' is called.";
+        },
+
+        get value() {
+        },
+        set value(val) {
+        }
+    };
+
+    function test(object, property) {
+        object[property];
+    }
+    noInline(test);
+
+    for (var i = 0; i < 10000; ++i)
+        test(object, property);
+
+    shouldThrow(function () { test(object, propertyKey); }, "Error: propertyKey.toString is called.");
+    if (called)
+        throw new Error(called);
+    toStringCalled = false;
+    shouldThrow(function () { test(null, propertyKey); }, "TypeError: null is not an object (evaluating 'object[property]')");
+    if (toStringCalled)
+        throw new Error("toString is called.");
+}());
+
+
+// GetByValString.
+(function () {
+    var called;
+    var toStringCalled = false;
+    var property = {
+        toString() {
+            toStringCalled = true;
+            return "value";
+        }
+    };
+    function test(array, length, property) {
+        var result = 0;
+        for (var i = 0; i < length; ++i)
+            result += array[property];
+        return result;
+    }
+    noInline(test);
+
+    Object.defineProperty(String.prototype, "", {
+        get: function () {
+            called = "getter for '' is called.";
+        }
+    });
+
+    var array = [1, 2, 3];
+    for (var i = 0; i < 100000; ++i)
+        test(array, array.length, 0);
+
+    var array = [1, false, 3];
+    for (var i = 0; i < 100000; ++i)
+        test(array, array.length, 1);
+
+    test("hello", "hello".length, 2);
+    shouldThrow(function () { test("hello", "hello".length, propertyKey); }, "Error: propertyKey.toString is called.");
+    if (called)
+        throw new Error(called);
+    toStringCalled = false;
+    shouldThrow(function () { test(null, 20, propertyKey); }, "TypeError: null is not an object (near '...for (var i = 0; i < length; ++i)...')");
+    if (toStringCalled)
+        throw new Error("toString is called.");
+}());