[JSC] op_has_indexed_property should not assume subscript part is Uint32
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2019 06:35:17 +0000 (06:35 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 12 Apr 2019 06:35:17 +0000 (06:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196850

Reviewed by Saam Barati.

JSTests:

* stress/has-indexed-property-should-accept-non-int32.js: Added.
(foo):

Source/JavaScriptCore:

op_has_indexed_property assumed that subscript part is always Uint32. However, this is just a load from non-constant RegisterID,
DFG can store it in double format and can perform OSR exit. op_has_indexed_property should not assume that.
In this patch, instead, we check it with isAnyInt and get uint32_t from AnyInt.

* jit/JITOpcodes.cpp:
(JSC::JIT::emit_op_has_indexed_property):
* jit/JITOpcodes32_64.cpp:
(JSC::JIT::emit_op_has_indexed_property):
* jit/JITOperations.cpp:
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):

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

JSTests/ChangeLog
JSTests/stress/has-indexed-property-should-accept-non-int32.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/JITOpcodes.cpp
Source/JavaScriptCore/jit/JITOpcodes32_64.cpp
Source/JavaScriptCore/jit/JITOperations.cpp
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
Source/JavaScriptCore/runtime/JSCJSValue.h
Source/JavaScriptCore/runtime/JSCJSValueInlines.h
Source/JavaScriptCore/runtime/JSGlobalObject.cpp

index 7bdded8..bef9be9 100644 (file)
@@ -1,3 +1,13 @@
+2019-04-11  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] op_has_indexed_property should not assume subscript part is Uint32
+        https://bugs.webkit.org/show_bug.cgi?id=196850
+
+        Reviewed by Saam Barati.
+
+        * stress/has-indexed-property-should-accept-non-int32.js: Added.
+        (foo):
+
 2019-04-11  Saam barati  <sbarati@apple.com>
 
         Remove invalid assertion in operationInstanceOfCustom
diff --git a/JSTests/stress/has-indexed-property-should-accept-non-int32.js b/JSTests/stress/has-indexed-property-should-accept-non-int32.js
new file mode 100644 (file)
index 0000000..5e5486b
--- /dev/null
@@ -0,0 +1,13 @@
+//@ runDefault("--useRandomizingFuzzerAgent=1", "--jitPolicyScale=0", "--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
+function foo(obj) {
+  for (var x in obj) {
+    if (0 > 0) {
+      break;
+    }
+  }
+  0 && Object.prototype.hasOwnProperty
+}
+
+foo([]);
+foo([]);
+foo([0, 0]);
index e7b3f98..25f7144 100644 (file)
@@ -1,3 +1,22 @@
+2019-04-11  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] op_has_indexed_property should not assume subscript part is Uint32
+        https://bugs.webkit.org/show_bug.cgi?id=196850
+
+        Reviewed by Saam Barati.
+
+        op_has_indexed_property assumed that subscript part is always Uint32. However, this is just a load from non-constant RegisterID,
+        DFG can store it in double format and can perform OSR exit. op_has_indexed_property should not assume that.
+        In this patch, instead, we check it with isAnyInt and get uint32_t from AnyInt.
+
+        * jit/JITOpcodes.cpp:
+        (JSC::JIT::emit_op_has_indexed_property):
+        * jit/JITOpcodes32_64.cpp:
+        (JSC::JIT::emit_op_has_indexed_property):
+        * jit/JITOperations.cpp:
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::SLOW_PATH_DECL):
+
 2019-04-11  Saam barati  <sbarati@apple.com>
 
         Remove invalid assertion in operationInstanceOfCustom
index e65851c..18ceba6 100644 (file)
@@ -1249,6 +1249,8 @@ void JIT::emit_op_has_indexed_property(const Instruction* currentInstruction)
     
     emitGetVirtualRegisters(base, regT0, property, regT1);
 
+    emitJumpSlowCaseIfNotInt(regT1);
+
     // This is technically incorrect - we're zero-extending an int32. On the hot path this doesn't matter.
     // We check the value as if it was a uint32 against the m_vectorLength - which will always fail if
     // number was signed since m_vectorLength is always less than intmax (since the total allocation
index 0816da4..25a9c8f 100644 (file)
@@ -1121,7 +1121,8 @@ void JIT::emit_op_has_indexed_property(const Instruction* currentInstruction)
     emitLoadPayload(base, regT0);
     emitJumpSlowCaseIfNotJSCell(base);
 
-    emitLoadPayload(property, regT1);
+    emitLoad(property, regT3, regT1);
+    addSlowCase(branchIfNotInt32(regT3));
 
     // This is technically incorrect - we're zero-extending an int32. On the hot path this doesn't matter.
     // We check the value as if it was a uint32 against the m_vectorLength - which will always fail if
index cedd5bf..d3d8226 100644 (file)
@@ -2013,7 +2013,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyDefault(ExecState* exec,
     JSValue subscript = JSValue::decode(encodedSubscript);
     
     ASSERT(baseValue.isObject());
-    ASSERT(subscript.isUInt32());
+    ASSERT(subscript.isUInt32AsAnyInt());
 
     JSObject* object = asObject(baseValue);
     bool didOptimize = false;
@@ -2043,7 +2043,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyDefault(ExecState* exec,
         }
     }
 
-    uint32_t index = subscript.asUInt32();
+    uint32_t index = subscript.asUInt32AsAnyInt();
     if (object->canGetIndexQuickly(index))
         return JSValue::encode(JSValue(JSValue::JSTrue));
 
@@ -2064,10 +2064,10 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyGeneric(ExecState* exec,
     JSValue subscript = JSValue::decode(encodedSubscript);
     
     ASSERT(baseValue.isObject());
-    ASSERT(subscript.isUInt32());
+    ASSERT(subscript.isUInt32AsAnyInt());
 
     JSObject* object = asObject(baseValue);
-    uint32_t index = subscript.asUInt32();
+    uint32_t index = subscript.asUInt32AsAnyInt();
     if (object->canGetIndexQuickly(index))
         return JSValue::encode(JSValue(JSValue::JSTrue));
 
@@ -2077,7 +2077,7 @@ EncodedJSValue JIT_OPERATION operationHasIndexedPropertyGeneric(ExecState* exec,
         // https://bugs.webkit.org/show_bug.cgi?id=149886
         byValInfo->arrayProfile->setOutOfBounds();
     }
-    return JSValue::encode(jsBoolean(object->hasPropertyGeneric(exec, subscript.asUInt32(), PropertySlot::InternalMethodType::GetOwnProperty)));
+    return JSValue::encode(jsBoolean(object->hasPropertyGeneric(exec, index, PropertySlot::InternalMethodType::GetOwnProperty)));
 }
     
 EncodedJSValue JIT_OPERATION operationGetByValString(ExecState* exec, EncodedJSValue encodedBase, EncodedJSValue encodedSubscript, ByValInfo* byValInfo)
index c2bc131..43eec84 100644 (file)
@@ -1530,8 +1530,8 @@ EncodedJSValue JSC_HOST_CALL arrayProtoPrivateFuncAppendMemcpy(ExecState* exec)
     JSArray* resultArray = jsCast<JSArray*>(exec->uncheckedArgument(0));
     JSArray* otherArray = jsCast<JSArray*>(exec->uncheckedArgument(1));
     JSValue startValue = exec->uncheckedArgument(2);
-    ASSERT(startValue.isAnyInt() && startValue.asAnyInt() >= 0 && startValue.asAnyInt() <= std::numeric_limits<unsigned>::max());
-    unsigned startIndex = static_cast<unsigned>(startValue.asAnyInt());
+    ASSERT(startValue.isUInt32AsAnyInt());
+    unsigned startIndex = startValue.asUInt32AsAnyInt();
     bool success = resultArray->appendMemcpy(exec, vm, startIndex, otherArray);
     EXCEPTION_ASSERT(!scope.exception() || !success);
     if (success)
index 59cb50f..8544188 100644 (file)
@@ -903,8 +903,8 @@ SLOW_PATH_DECL(slow_path_has_indexed_property)
     CHECK_EXCEPTION();
     JSValue property = GET(bytecode.m_property).jsValue();
     metadata.m_arrayProfile.observeStructure(base->structure(vm));
-    ASSERT(property.isUInt32());
-    RETURN(jsBoolean(base->hasPropertyGeneric(exec, property.asUInt32(), PropertySlot::InternalMethodType::GetOwnProperty)));
+    ASSERT(property.isUInt32AsAnyInt());
+    RETURN(jsBoolean(base->hasPropertyGeneric(exec, property.asUInt32AsAnyInt(), PropertySlot::InternalMethodType::GetOwnProperty)));
 }
 
 SLOW_PATH_DECL(slow_path_has_structure_property)
@@ -996,11 +996,8 @@ SLOW_PATH_DECL(slow_path_to_index_string)
     BEGIN();
     auto bytecode = pc->as<OpToIndexString>();
     JSValue indexValue = GET(bytecode.m_index).jsValue();
-    ASSERT(indexValue.isAnyInt());
-    ASSERT(indexValue.asAnyInt() <= UINT32_MAX);
-    ASSERT(indexValue.asAnyInt() >= 0);
-    uint32_t index = static_cast<uint32_t>(indexValue.asAnyInt());
-    RETURN(jsString(exec, Identifier::from(exec, index).string()));
+    ASSERT(indexValue.isUInt32AsAnyInt());
+    RETURN(jsString(exec, Identifier::from(exec, indexValue.asUInt32AsAnyInt()).string()));
 }
 
 SLOW_PATH_DECL(slow_path_profile_type_clear_log)
index 22d2050..ef7033e 100644 (file)
@@ -211,6 +211,8 @@ public:
     int32_t asInt32() const;
     uint32_t asUInt32() const;
     int64_t asAnyInt() const;
+    uint32_t asUInt32AsAnyInt() const;
+    int32_t asInt32AsAnyInt() const;
     double asDouble() const;
     bool asBoolean() const;
     double asNumber() const;
@@ -228,6 +230,8 @@ public:
     bool isUndefinedOrNull() const;
     bool isBoolean() const;
     bool isAnyInt() const;
+    bool isUInt32AsAnyInt() const;
+    bool isInt32AsAnyInt() const;
     bool isNumber() const;
     bool isString() const;
     bool isBigInt() const;
index 6cf3a22..0d3324a 100644 (file)
@@ -571,6 +571,38 @@ inline int64_t JSValue::asAnyInt() const
     return static_cast<int64_t>(asDouble());
 }
 
+inline bool JSValue::isInt32AsAnyInt() const
+{
+    if (!isAnyInt())
+        return false;
+    int64_t value = asAnyInt();
+    return value >= INT32_MIN && value <= INT32_MAX;
+}
+
+inline int32_t JSValue::asInt32AsAnyInt() const
+{
+    ASSERT(isInt32AsAnyInt());
+    if (isInt32())
+        return asInt32();
+    return static_cast<int32_t>(asDouble());
+}
+
+inline bool JSValue::isUInt32AsAnyInt() const
+{
+    if (!isAnyInt())
+        return false;
+    int64_t value = asAnyInt();
+    return value >= 0 && value <= UINT32_MAX;
+}
+
+inline uint32_t JSValue::asUInt32AsAnyInt() const
+{
+    ASSERT(isUInt32AsAnyInt());
+    if (isUInt32())
+        return asUInt32();
+    return static_cast<uint32_t>(asDouble());
+}
+
 inline bool JSValue::isString() const
 {
     return isCell() && asCell()->isString();
index 9302701..64d468e 100644 (file)
@@ -248,11 +248,8 @@ static EncodedJSValue JSC_HOST_CALL makeBoundFunction(ExecState* exec)
     JSValue lengthValue = exec->uncheckedArgument(3);
     JSString* nameString = asString(exec->uncheckedArgument(4));
 
-    ASSERT(lengthValue.isAnyInt());
-    ASSERT(lengthValue.asAnyInt() <= INT32_MAX);
-    ASSERT(lengthValue.asAnyInt() >= INT32_MIN);
-    int32_t length = lengthValue.toInt32(exec);
-    scope.assertNoException();
+    ASSERT(lengthValue.isInt32AsAnyInt());
+    int32_t length = lengthValue.asInt32AsAnyInt();
 
     String name = nameString->value(exec);
     RETURN_IF_EXCEPTION(scope, { });