[JSC] Keep JSString::value(ExecState*)'s result as String instead of `const String&`
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 28 Sep 2019 03:40:23 +0000 (03:40 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 28 Sep 2019 03:40:23 +0000 (03:40 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202330

Reviewed by Saam Barati.

JSTests:

* stress/to-lower-case-gc-stress.js: Added.

Source/JavaScriptCore:

In toLocaleLowerCase and toLocaleUpperCase, we get `const String&` from JSString* and use it.
But if this string is newly created one in toLocaleLowerCase and toLocaleUpperCase (like, passing a number, and number.toString() is called
in C++), after getting `const String&`, our C++ code potentially does not have any reference to the owner of this `const String&`. So, this
JSString* can be collected by GC, while `const String&` is used. This makes `const String&` destroyed, and causes crash.

In this patch, we receive it as `String` instead of `const String&` to ref it. This ensures that this string is live even if the owner is collected.
I grepped the source code and make this changes conservatively to places which looks dangerous. And I added error checks more after calling `value(exec)`.

In this patch, I didn't introduce the change like that: `JSString::value(ExecState*)` returns `String` instead of `const String&`. Some of places are
really performance sensitive and we want to use the current behavior when we can ensure the owners are alive. We could figure out these points, and we
can change the default behavior of `JSString::value` function to returning `String`. But for now, I plan it as a future work.

* dfg/DFGOperations.cpp:
* jsc.cpp:
(GlobalObject::moduleLoaderImportModule):
* runtime/DateConstructor.cpp:
(JSC::constructDate):
* runtime/JSCJSValueInlines.h:
(JSC::JSValue::equalSlowCaseInline):
* runtime/RegExpMatchesArray.h:
(JSC::createRegExpMatchesArray):
* runtime/StringPrototype.cpp:
(JSC::toLocaleCase):
(JSC::stringProtoFuncToLocaleLowerCase):
(JSC::stringProtoFuncToLocaleUpperCase):
* tools/JSDollarVM.cpp:
(JSC::functionCreateBuiltin):

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

JSTests/ChangeLog
JSTests/stress/to-lower-case-gc-stress.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/jsc.cpp
Source/JavaScriptCore/runtime/DateConstructor.cpp
Source/JavaScriptCore/runtime/JSCJSValueInlines.h
Source/JavaScriptCore/runtime/RegExpMatchesArray.h
Source/JavaScriptCore/runtime/StringPrototype.cpp
Source/JavaScriptCore/tools/JSDollarVM.cpp

index 60af16a..fcccf23 100644 (file)
@@ -1,3 +1,12 @@
+2019-09-27  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Keep JSString::value(ExecState*)'s result as String instead of `const String&`
+        https://bugs.webkit.org/show_bug.cgi?id=202330
+
+        Reviewed by Saam Barati.
+
+        * stress/to-lower-case-gc-stress.js: Added.
+
 2019-09-27  Alexey Shvayka  <shvaikalesh@gmail.com>
 
         Non-standard Error properties should not be enumerable
diff --git a/JSTests/stress/to-lower-case-gc-stress.js b/JSTests/stress/to-lower-case-gc-stress.js
new file mode 100644 (file)
index 0000000..483ecfe
--- /dev/null
@@ -0,0 +1,4 @@
+//@ runDefault("--sweepSynchronously=1", "--collectContinuously=1")
+
+for (var i = 0; i < 10000; i++)
+    ''.toLocaleLowerCase.call(-1, 0);
index b5860a0..960732a 100644 (file)
@@ -1,3 +1,38 @@
+2019-09-27  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Keep JSString::value(ExecState*)'s result as String instead of `const String&`
+        https://bugs.webkit.org/show_bug.cgi?id=202330
+
+        Reviewed by Saam Barati.
+
+        In toLocaleLowerCase and toLocaleUpperCase, we get `const String&` from JSString* and use it.
+        But if this string is newly created one in toLocaleLowerCase and toLocaleUpperCase (like, passing a number, and number.toString() is called
+        in C++), after getting `const String&`, our C++ code potentially does not have any reference to the owner of this `const String&`. So, this
+        JSString* can be collected by GC, while `const String&` is used. This makes `const String&` destroyed, and causes crash.
+
+        In this patch, we receive it as `String` instead of `const String&` to ref it. This ensures that this string is live even if the owner is collected.
+        I grepped the source code and make this changes conservatively to places which looks dangerous. And I added error checks more after calling `value(exec)`.
+
+        In this patch, I didn't introduce the change like that: `JSString::value(ExecState*)` returns `String` instead of `const String&`. Some of places are
+        really performance sensitive and we want to use the current behavior when we can ensure the owners are alive. We could figure out these points, and we
+        can change the default behavior of `JSString::value` function to returning `String`. But for now, I plan it as a future work.
+
+        * dfg/DFGOperations.cpp:
+        * jsc.cpp:
+        (GlobalObject::moduleLoaderImportModule):
+        * runtime/DateConstructor.cpp:
+        (JSC::constructDate):
+        * runtime/JSCJSValueInlines.h:
+        (JSC::JSValue::equalSlowCaseInline):
+        * runtime/RegExpMatchesArray.h:
+        (JSC::createRegExpMatchesArray):
+        * runtime/StringPrototype.cpp:
+        (JSC::toLocaleCase):
+        (JSC::stringProtoFuncToLocaleLowerCase):
+        (JSC::stringProtoFuncToLocaleUpperCase):
+        * tools/JSDollarVM.cpp:
+        (JSC::functionCreateBuiltin):
+
 2019-09-27  Keith Miller  <keith_miller@apple.com>
 
         OSR exit shouldn't bother updating get_by_id array profiles that have changed modes
index e0f7082..082c8db 100644 (file)
@@ -2292,7 +2292,7 @@ JSString* JIT_OPERATION operationToLowerCase(ExecState* exec, JSString* string,
 
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    const String& inputString = string->value(exec);
+    String inputString = string->value(exec);
     RETURN_IF_EXCEPTION(scope, nullptr);
     if (!inputString.length())
         return vm.smallStrings.emptyString();
index 71777d7..cb4abaf 100644 (file)
@@ -841,8 +841,9 @@ JSInternalPromise* GlobalObject::moduleLoaderImportModule(JSGlobalObject* global
     if (sourceOrigin.isNull())
         return reject(createError(exec, "Could not resolve the module specifier."_s));
 
-    const auto& referrer = sourceOrigin.string();
-    const auto& moduleName = moduleNameValue->value(exec);
+    auto referrer = sourceOrigin.string();
+    auto moduleName = moduleNameValue->value(exec);
+    RETURN_IF_EXCEPTION(throwScope, nullptr);
     if (UNLIKELY(catchScope.exception()))
         return reject(catchScope.exception());
 
index 239827a..f6b8fee 100644 (file)
@@ -133,7 +133,9 @@ JSObject* constructDate(ExecState* exec, JSGlobalObject* globalObject, JSValue n
             JSValue primitive = arg0.toPrimitive(exec);
             RETURN_IF_EXCEPTION(scope, nullptr);
             if (primitive.isString()) {
-                value = parseDate(exec, vm, asString(primitive)->value(exec));
+                String primitiveString = asString(primitive)->value(exec);
+                RETURN_IF_EXCEPTION(scope, nullptr);
+                value = parseDate(exec, vm, primitiveString);
                 RETURN_IF_EXCEPTION(scope, nullptr);
             } else
                 value = primitive.toNumber(exec);
index 16b0809..d59096e 100644 (file)
@@ -1030,7 +1030,9 @@ ALWAYS_INLINE bool JSValue::equalSlowCaseInline(ExecState* exec, JSValue v1, JSV
             RELEASE_AND_RETURN(scope, asString(v1)->equal(exec, asString(v2)));
 
         if (v1.isBigInt() && s2) {
-            JSBigInt* n = JSBigInt::stringToBigInt(exec, asString(v2)->value(exec));
+            String v2String = asString(v2)->value(exec);
+            RETURN_IF_EXCEPTION(scope, false);
+            JSBigInt* n = JSBigInt::stringToBigInt(exec, v2String);
             RETURN_IF_EXCEPTION(scope, false);
             if (!n)
                 return false;
@@ -1040,7 +1042,9 @@ ALWAYS_INLINE bool JSValue::equalSlowCaseInline(ExecState* exec, JSValue v1, JSV
         }
 
         if (s1 && v2.isBigInt()) {
-            JSBigInt* n = JSBigInt::stringToBigInt(exec, asString(v1)->value(exec));
+            String v1String = asString(v1)->value(exec);
+            RETURN_IF_EXCEPTION(scope, false);
+            JSBigInt* n = JSBigInt::stringToBigInt(exec, v1String);
             RETURN_IF_EXCEPTION(scope, false);
             if (!n)
                 return false;
index e0913d5..eb2baa3 100644 (file)
@@ -167,8 +167,14 @@ ALWAYS_INLINE JSArray* createRegExpMatchesArray(
 
 inline JSArray* createRegExpMatchesArray(ExecState* exec, JSGlobalObject* globalObject, JSString* string, RegExp* regExp, unsigned startOffset)
 {
+    VM& vm = globalObject->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     MatchResult ignoredResult;
-    return createRegExpMatchesArray(globalObject->vm(), globalObject, string, string->value(exec), regExp, startOffset, ignoredResult);
+    String input = string->value(exec);
+    RETURN_IF_EXCEPTION(scope, { });
+
+    RELEASE_AND_RETURN(scope, createRegExpMatchesArray(vm, globalObject, string, input, regExp, startOffset, ignoredResult));
 }
 JSArray* createEmptyRegExpMatchesArray(JSGlobalObject*, JSString*, RegExp*);
 Structure* createRegExpMatchesArrayStructure(VM&, JSGlobalObject*);
index f880d54..5fa951a 100644 (file)
@@ -1520,11 +1520,22 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncLocaleCompare(ExecState* exec)
 }
 
 #if ENABLE(INTL)
-static EncodedJSValue toLocaleCase(ExecState* state, int32_t (*convertCase)(UChar*, int32_t, const UChar*, int32_t, const char*, UErrorCode*))
+enum class CaseConversionMode {
+    Upper,
+    Lower,
+};
+template<CaseConversionMode mode>
+static EncodedJSValue toLocaleCase(ExecState* state)
 {
     VM& vm = state->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
+    auto convertCase = [&] (auto&&... args) {
+        if (mode == CaseConversionMode::Lower)
+            return u_strToLower(std::forward<decltype(args)>(args)...);
+        return u_strToUpper(std::forward<decltype(args)>(args)...);
+    };
+
     // 1. Let O be RequireObjectCoercible(this value).
     JSValue thisValue = state->thisValue();
     if (!checkObjectCoercible(thisValue))
@@ -1533,7 +1544,7 @@ static EncodedJSValue toLocaleCase(ExecState* state, int32_t (*convertCase)(UCha
     // 2. Let S be ToString(O).
     JSString* sVal = thisValue.toString(state);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
-    const String& s = sVal->value(state);
+    String s = sVal->value(state);
 
     // 3. ReturnIfAbrupt(S).
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
@@ -1608,7 +1619,7 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncToLocaleLowerCase(ExecState* state)
 {
     // 13.1.2 String.prototype.toLocaleLowerCase ([locales])
     // http://ecma-international.org/publications/standards/Ecma-402.htm
-    return toLocaleCase(state, u_strToLower);
+    return toLocaleCase<CaseConversionMode::Lower>(state);
 }
 
 EncodedJSValue JSC_HOST_CALL stringProtoFuncToLocaleUpperCase(ExecState* state)
@@ -1616,7 +1627,7 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncToLocaleUpperCase(ExecState* state)
     // 13.1.3 String.prototype.toLocaleUpperCase ([locales])
     // http://ecma-international.org/publications/standards/Ecma-402.htm
     // This function interprets a string value as a sequence of code points, as described in ES2015, 6.1.4. This function behaves in exactly the same way as String.prototype.toLocaleLowerCase, except that characters are mapped to their uppercase equivalents as specified in the Unicode character database.
-    return toLocaleCase(state, u_strToUpper);
+    return toLocaleCase<CaseConversionMode::Upper>(state);
 }
 #endif // ENABLE(INTL)
 
index 3438c4e..6f9fafd 100644 (file)
@@ -2049,7 +2049,7 @@ static EncodedJSValue JSC_HOST_CALL functionCreateBuiltin(ExecState* exec)
     String functionText = asString(exec->argument(0))->value(exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
-    const SourceCode& source = makeSource(functionText, { });
+    SourceCode source = makeSource(functionText, { });
     JSFunction* func = JSFunction::create(vm, createBuiltinExecutable(vm, source, Identifier::fromString(vm, "foo"), ConstructorKind::None, ConstructAbility::CannotConstruct)->link(vm, nullptr, source), exec->lexicalGlobalObject());
 
     return JSValue::encode(func);