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): (.): Canonical link: https://commits.webkit.org/161163@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182057 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Mar 27, 2015 · 0c6f394 · 0c6f394
1 parent a4cf1c4
commit 0c6f394
Show file tree

Hide file tree

Showing 10 changed files with 344 additions and 9 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -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

diff --git a/Source/JavaScriptCore/dfg/DFGOperations.cpp b/Source/JavaScriptCore/dfg/DFGOperations.cpp
@@ -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));
 }
 

diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
@@ -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);
     }
 

diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
@@ -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);
 }
 

diff --git a/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp b/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
@@ -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)

diff --git a/Source/JavaScriptCore/runtime/JSCJSValue.h b/Source/JavaScriptCore/runtime/JSCJSValue.h
@@ -302,6 +302,7 @@ class JSValue {
     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.

diff --git a/Source/JavaScriptCore/runtime/JSCJSValueInlines.h b/Source/JavaScriptCore/runtime/JSCJSValueInlines.h
@@ -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

diff --git a/Source/JavaScriptCore/runtime/ObjectPrototype.cpp b/Source/JavaScriptCore/runtime/ObjectPrototype.cpp
@@ -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/...re/tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js b/...re/tests/stress/exception-in-to-property-key-should-be-handled-early-in-object-methods.js
@@ -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.");