Some of JSStringView::SafeView methods are not idiomatically safe for JSString to...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Nov 2016 19:42:41 +0000 (19:42 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 14 Nov 2016 19:42:41 +0000 (19:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164701
<rdar://problem/27462104>

Reviewed by Darin Adler.

JSTests:

* stress/string-prototype-charCodeAt-on-too-long-rope.js: Added.

Source/JavaScriptCore:

The characters8(), characters16(), and operator[] in JSString::SafeView converts
the underlying JSString to a StringView via get(), and then uses the StringView
without first checking if an exception was thrown during the conversion.  This is
unsafe because the conversion may have failed.

Instead, we should remove these 3 convenience methods, and make the caller
explicitly call get() and do the appropriate exception checks before using the
StringView.

* runtime/JSGlobalObjectFunctions.cpp:
(JSC::toStringView):
(JSC::encode):
(JSC::decode):
(JSC::globalFuncParseInt):
(JSC::globalFuncEscape):
(JSC::globalFuncUnescape):
(JSC::toSafeView): Deleted.
* runtime/JSONObject.cpp:
(JSC::JSONProtoFuncParse):
* runtime/JSString.h:
(JSC::JSString::SafeView::length):
(JSC::JSString::SafeView::characters8): Deleted.
(JSC::JSString::SafeView::characters16): Deleted.
(JSC::JSString::SafeView::operator[]): Deleted.
* runtime/StringPrototype.cpp:
(JSC::stringProtoFuncRepeatCharacter):
(JSC::stringProtoFuncCharAt):
(JSC::stringProtoFuncCharCodeAt):
(JSC::stringProtoFuncNormalize):

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

JSTests/ChangeLog
JSTests/stress/string-prototype-charCodeAt-on-too-long-rope.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp
Source/JavaScriptCore/runtime/JSONObject.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/JavaScriptCore/runtime/StringPrototype.cpp

index bee3187..111df85 100644 (file)
@@ -1,5 +1,15 @@
 2016-11-14  Mark Lam  <mark.lam@apple.com>
 
+        Some of JSStringView::SafeView methods are not idiomatically safe for JSString to StringView conversions.
+        https://bugs.webkit.org/show_bug.cgi?id=164701
+        <rdar://problem/27462104>
+
+        Reviewed by Darin Adler.
+
+        * stress/string-prototype-charCodeAt-on-too-long-rope.js: Added.
+
+2016-11-14  Mark Lam  <mark.lam@apple.com>
+
         RegExpObject::exec/match should handle errors gracefully.
         https://bugs.webkit.org/show_bug.cgi?id=155145
         <rdar://problem/27435934>
diff --git a/JSTests/stress/string-prototype-charCodeAt-on-too-long-rope.js b/JSTests/stress/string-prototype-charCodeAt-on-too-long-rope.js
new file mode 100644 (file)
index 0000000..3d28226
--- /dev/null
@@ -0,0 +1,32 @@
+//@ if $buildType == "debug" then runFTLNoCJIT("--maxSingleAllocationSize=20000000") else skip end
+
+function shouldEqual(actual, expected) {
+    if (actual != expected) {
+        throw "ERROR: expect " + expected + ", actual " + actual;
+    }
+}
+
+s0 = "";
+s1 = "NaNxxxxx";
+
+try {
+    for (var count = 0; count < 27; count++) {
+        var oldS1 = s1;
+        s1 += s1;
+        s0 = oldS1;
+    }
+} catch (e) { }
+
+try {
+    s1 += s0;
+} catch (e) { }
+
+var exception;
+try {
+    for (var v of s1) { }
+} catch (e) {
+    exception = e;
+}
+
+shouldEqual(exception, "Error: Out of memory");
+
index 62c022f..54ce0cc 100644 (file)
@@ -1,5 +1,43 @@
 2016-11-14  Mark Lam  <mark.lam@apple.com>
 
+        Some of JSStringView::SafeView methods are not idiomatically safe for JSString to StringView conversions.
+        https://bugs.webkit.org/show_bug.cgi?id=164701
+        <rdar://problem/27462104>
+
+        Reviewed by Darin Adler.
+
+        The characters8(), characters16(), and operator[] in JSString::SafeView converts
+        the underlying JSString to a StringView via get(), and then uses the StringView
+        without first checking if an exception was thrown during the conversion.  This is
+        unsafe because the conversion may have failed.
+        
+        Instead, we should remove these 3 convenience methods, and make the caller
+        explicitly call get() and do the appropriate exception checks before using the
+        StringView.
+
+        * runtime/JSGlobalObjectFunctions.cpp:
+        (JSC::toStringView):
+        (JSC::encode):
+        (JSC::decode):
+        (JSC::globalFuncParseInt):
+        (JSC::globalFuncEscape):
+        (JSC::globalFuncUnescape):
+        (JSC::toSafeView): Deleted.
+        * runtime/JSONObject.cpp:
+        (JSC::JSONProtoFuncParse):
+        * runtime/JSString.h:
+        (JSC::JSString::SafeView::length):
+        (JSC::JSString::SafeView::characters8): Deleted.
+        (JSC::JSString::SafeView::characters16): Deleted.
+        (JSC::JSString::SafeView::operator[]): Deleted.
+        * runtime/StringPrototype.cpp:
+        (JSC::stringProtoFuncRepeatCharacter):
+        (JSC::stringProtoFuncCharAt):
+        (JSC::stringProtoFuncCharCodeAt):
+        (JSC::stringProtoFuncNormalize):
+
+2016-11-14  Mark Lam  <mark.lam@apple.com>
+
         RegExpObject::exec/match should handle errors gracefully.
         https://bugs.webkit.org/show_bug.cgi?id=155145
         <rdar://problem/27435934>
index 1bd4f08..577b1ad 100644 (file)
@@ -57,13 +57,17 @@ namespace JSC {
 static const char* const ObjectProtoCalledOnNullOrUndefinedError = "Object.prototype.__proto__ called on null or undefined";
 
 template<typename CallbackWhenNoException>
-static ALWAYS_INLINE typename std::result_of<CallbackWhenNoException(JSString::SafeView&)>::type toSafeView(ExecState* exec, JSValue value, CallbackWhenNoException callback)
+static ALWAYS_INLINE typename std::result_of<CallbackWhenNoException(StringView)>::type toStringView(ExecState* exec, JSValue value, CallbackWhenNoException callback)
 {
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
     JSString* string = value.toStringOrNull(exec);
     if (UNLIKELY(!string))
         return { };
     JSString::SafeView view = string->view(exec);
-    return callback(view);
+    StringView stringView = view.get();
+    RETURN_IF_EXCEPTION(scope, { });
+    return callback(stringView);
 }
 
 template<unsigned charactersCount>
@@ -158,7 +162,7 @@ static JSValue encode(ExecState* exec, const Bitmap<256>& doNotEscape, const Cha
 
 static JSValue encode(ExecState* exec, const Bitmap<256>& doNotEscape)
 {
-    return toSafeView(exec, exec->argument(0), [&] (JSString::SafeView& view) {
+    return toStringView(exec, exec->argument(0), [&] (StringView view) {
         if (view.is8Bit())
             return encode(exec, doNotEscape, view.characters8(), view.length());
         return encode(exec, doNotEscape, view.characters16(), view.length());
@@ -236,7 +240,7 @@ static JSValue decode(ExecState* exec, const CharType* characters, int length, c
 
 static JSValue decode(ExecState* exec, const Bitmap<256>& doNotUnescape, bool strict)
 {
-    return toSafeView(exec, exec->argument(0), [&] (JSString::SafeView& view) {
+    return toStringView(exec, exec->argument(0), [&] (StringView view) {
         if (view.is8Bit())
             return decode(exec, view.characters8(), view.length(), doNotUnescape, strict);
         return decode(exec, view.characters16(), view.length(), doNotUnescape, strict);
@@ -707,8 +711,8 @@ EncodedJSValue JSC_HOST_CALL globalFuncParseInt(ExecState* exec)
     }
 
     // If ToString throws, we shouldn't call ToInt32.
-    return toSafeView(exec, value, [&] (JSString::SafeView& view) {
-        return JSValue::encode(jsNumber(parseInt(view.get(), radixValue.toInt32(exec))));
+    return toStringView(exec, value, [&] (StringView view) {
+        return JSValue::encode(jsNumber(parseInt(view, radixValue.toInt32(exec))));
     });
 }
 
@@ -765,7 +769,7 @@ EncodedJSValue JSC_HOST_CALL globalFuncEscape(ExecState* exec)
         "*+-./@_"
     );
 
-    return JSValue::encode(toSafeView(exec, exec->argument(0), [&] (JSString::SafeView& view) {
+    return JSValue::encode(toStringView(exec, exec->argument(0), [&] (StringView view) {
         JSStringBuilder builder;
         if (view.is8Bit()) {
             const LChar* c = view.characters8();
@@ -804,7 +808,7 @@ EncodedJSValue JSC_HOST_CALL globalFuncEscape(ExecState* exec)
 
 EncodedJSValue JSC_HOST_CALL globalFuncUnescape(ExecState* exec)
 {
-    return JSValue::encode(toSafeView(exec, exec->argument(0), [&] (JSString::SafeView& view) {
+    return JSValue::encode(toStringView(exec, exec->argument(0), [&] (StringView view) {
         StringBuilder builder;
         int k = 0;
         int len = view.length();
index bbf92b1..b215130 100644 (file)
@@ -763,16 +763,18 @@ EncodedJSValue JSC_HOST_CALL JSONProtoFuncParse(ExecState* exec)
         return throwVMError(exec, scope, createError(exec, ASCIILiteral("JSON.parse requires at least one parameter")));
     JSString::SafeView source = exec->uncheckedArgument(0).toString(exec)->view(exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = source.get();
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     JSValue unfiltered;
     LocalScope localScope(vm);
-    if (source.is8Bit()) {
-        LiteralParser<LChar> jsonParser(exec, source.characters8(), source.length(), StrictJSON);
+    if (view.is8Bit()) {
+        LiteralParser<LChar> jsonParser(exec, view.characters8(), view.length(), StrictJSON);
         unfiltered = jsonParser.tryLiteralParse();
         if (!unfiltered)
             return throwVMError(exec, scope, createSyntaxError(exec, jsonParser.getErrorMessage()));
     } else {
-        LiteralParser<UChar> jsonParser(exec, source.characters16(), source.length(), StrictJSON);
+        LiteralParser<UChar> jsonParser(exec, view.characters16(), view.length(), StrictJSON);
         unfiltered = jsonParser.tryLiteralParse();
         if (!unfiltered)
             return throwVMError(exec, scope, createSyntaxError(exec, jsonParser.getErrorMessage()));
index 143c4bc..8115e20 100644 (file)
@@ -482,9 +482,6 @@ public:
 
     bool is8Bit() const { return m_string->is8Bit(); }
     unsigned length() const { return m_string->length(); }
-    const LChar* characters8() const { return get().characters8(); }
-    const UChar* characters16() const { return get().characters16(); }
-    UChar operator[](unsigned index) const { return get()[index]; }
 
 private:
     ExecState& m_state;
index a1645bc..6802881 100644 (file)
@@ -791,6 +791,9 @@ static inline JSString* repeatCharacter(ExecState& exec, CharacterType character
 
 EncodedJSValue JSC_HOST_CALL stringProtoFuncRepeatCharacter(ExecState* exec)
 {
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
     // For a string which length is single, instead of creating ropes,
     // allocating a sequential buffer and fill with the repeated string for efficiency.
     ASSERT(exec->argumentCount() == 2);
@@ -802,18 +805,18 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncRepeatCharacter(ExecState* exec)
     JSValue repeatCountValue = exec->uncheckedArgument(1);
     RELEASE_ASSERT(repeatCountValue.isNumber());
     int32_t repeatCount;
-    {
-        VM& vm = exec->vm();
-        auto scope = DECLARE_THROW_SCOPE(vm);
-        double value = repeatCountValue.asNumber();
-        if (value > JSString::MaxLength)
-            return JSValue::encode(throwOutOfMemoryError(exec, scope));
-        repeatCount = static_cast<int32_t>(value);
-    }
+    double value = repeatCountValue.asNumber();
+    if (value > JSString::MaxLength)
+        return JSValue::encode(throwOutOfMemoryError(exec, scope));
+    repeatCount = static_cast<int32_t>(value);
     ASSERT(repeatCount >= 0);
     ASSERT(!repeatCountValue.isDouble() || repeatCountValue.asDouble() == repeatCount);
 
-    UChar character = string->view(exec)[0];
+    JSString::SafeView safeView = string->view(exec);
+    StringView view = safeView.get();
+    ASSERT(view.length() == 1 && !scope.exception());
+    UChar character = view[0];
+    scope.release();
     if (!(character & ~0xff))
         return JSValue::encode(repeatCharacter(*exec, static_cast<LChar>(character), repeatCount));
     return JSValue::encode(repeatCharacter(*exec, character, repeatCount));
@@ -904,16 +907,19 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncCharAt(ExecState* exec)
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
     JSString::SafeView string = thisValue.toString(exec)->view(exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = string.get();
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
     JSValue a0 = exec->argument(0);
     if (a0.isUInt32()) {
         uint32_t i = a0.asUInt32();
-        if (i < string.length())
-            return JSValue::encode(jsSingleCharacterString(exec, string[i]));
+        if (i < view.length())
+            return JSValue::encode(jsSingleCharacterString(exec, view[i]));
         return JSValue::encode(jsEmptyString(exec));
     }
     double dpos = a0.toInteger(exec);
-    if (dpos >= 0 && dpos < string.length())
-        return JSValue::encode(jsSingleCharacterString(exec, string[static_cast<unsigned>(dpos)]));
+    if (dpos >= 0 && dpos < view.length())
+        return JSValue::encode(jsSingleCharacterString(exec, view[static_cast<unsigned>(dpos)]));
     return JSValue::encode(jsEmptyString(exec));
 }
 
@@ -925,17 +931,21 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncCharCodeAt(ExecState* exec)
     JSValue thisValue = exec->thisValue();
     if (!checkObjectCoercible(thisValue))
         return throwVMTypeError(exec, scope);
-    JSString::SafeView string = thisValue.toString(exec)->view(exec);
+    JSString* jsString = thisValue.toString(exec);
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    JSString::SafeView string = jsString->view(exec);
+    StringView view = string.get();
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
     JSValue a0 = exec->argument(0);
     if (a0.isUInt32()) {
         uint32_t i = a0.asUInt32();
-        if (i < string.length())
-            return JSValue::encode(jsNumber(string[i]));
+        if (i < view.length())
+            return JSValue::encode(jsNumber(view[i]));
         return JSValue::encode(jsNaN());
     }
     double dpos = a0.toInteger(exec);
-    if (dpos >= 0 && dpos < string.length())
-        return JSValue::encode(jsNumber(string[static_cast<int>(dpos)]));
+    if (dpos >= 0 && dpos < view.length())
+        return JSValue::encode(jsNumber(view[static_cast<int>(dpos)]));
     return JSValue::encode(jsNaN());
 }
 
@@ -2008,6 +2018,8 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncNormalize(ExecState* exec)
         return throwVMTypeError(exec, scope);
     JSString::SafeView source = thisValue.toString(exec)->view(exec);
     RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    StringView view = source.get();
+    RETURN_IF_EXCEPTION(scope, encodedJSValue());
 
     UNormalizationMode form = UNORM_NFC;
     // Verify that the argument is provided and is not undefined.
@@ -2027,7 +2039,7 @@ EncodedJSValue JSC_HOST_CALL stringProtoFuncNormalize(ExecState* exec)
             return throwVMError(exec, scope, createRangeError(exec, ASCIILiteral("argument does not match any normalization form")));
     }
 
-    return JSValue::encode(normalize(exec, source.get().upconvertedCharacters(), source.length(), form));
+    return JSValue::encode(normalize(exec, view.upconvertedCharacters(), view.length(), form));
 }
 
 } // namespace JSC