Array.prototype.join should do overflow checks on string joins.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Sep 2016 23:41:56 +0000 (23:41 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Sep 2016 23:41:56 +0000 (23:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162459

Reviewed by Saam Barati.

JSTests:

* stress/array-join-on-strings-need-overflow-checks.js: Added.
(assert):
(catch):

Source/JavaScriptCore:

Change the 2 JSRopeString::create() functions that do joins to be private, and
force all clients of it to go through the jsString() utility functions that do
overflow checks before creating the ropes.

* dfg/DFGOperations.cpp:
* runtime/ArrayPrototype.cpp:
(JSC::slowJoin):
* runtime/JSString.h:
* runtime/Operations.h:
(JSC::jsString):

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

JSTests/ChangeLog
JSTests/stress/array-join-on-strings-need-overflow-checks.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOperations.cpp
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/JavaScriptCore/runtime/Operations.h

index 508b8c3..fcf04a7 100644 (file)
@@ -1,3 +1,14 @@
+2016-09-22  Mark Lam  <mark.lam@apple.com>
+
+        Array.prototype.join should do overflow checks on string joins.
+        https://bugs.webkit.org/show_bug.cgi?id=162459
+
+        Reviewed by Saam Barati.
+
+        * stress/array-join-on-strings-need-overflow-checks.js: Added.
+        (assert):
+        (catch):
+
 2016-09-22  Joseph Pecoraro  <pecoraro@apple.com>
 
         test262: Function length should be number of parameters before parameters with default values
diff --git a/JSTests/stress/array-join-on-strings-need-overflow-checks.js b/JSTests/stress/array-join-on-strings-need-overflow-checks.js
new file mode 100644 (file)
index 0000000..abe5f36
--- /dev/null
@@ -0,0 +1,33 @@
+function assert(x, y) {
+    if (x != y)
+        throw(" Expect: " + y + ", actual: " + x);
+}
+
+s1 = "";
+for (var k = 0; k < 2000; ++k)
+    s1 += "z";
+var expectedLength = 2000;
+assert(s1.length, 2000);
+
+s2 = 'x';
+expectedLength = 1;
+assert(s2.length, expectedLength);
+
+for (var i = 0; i < 22; ++i) {
+    expectedLength += expectedLength;
+    s2 += s2;
+    assert(s2.length, expectedLength);
+}
+
+var caughtException;
+try {
+    expectedLength = ((s1.length - 1) * s2.length) + 1;
+    result = Array.prototype.join.apply(s1, [s2]);
+    assert(result.length, expectedLength);
+} catch (e) {
+    caughtException = e;
+}
+
+if (!caughtException)
+    throw("Array.prototype.join should have thrown an exception when string length overflows");
+assert(caughtException, "Error: Out of memory");
index 3b355b0..d06f672 100644 (file)
@@ -1,3 +1,21 @@
+2016-09-22  Mark Lam  <mark.lam@apple.com>
+
+        Array.prototype.join should do overflow checks on string joins.
+        https://bugs.webkit.org/show_bug.cgi?id=162459
+
+        Reviewed by Saam Barati.
+
+        Change the 2 JSRopeString::create() functions that do joins to be private, and
+        force all clients of it to go through the jsString() utility functions that do
+        overflow checks before creating the ropes.
+
+        * dfg/DFGOperations.cpp:
+        * runtime/ArrayPrototype.cpp:
+        (JSC::slowJoin):
+        * runtime/JSString.h:
+        * runtime/Operations.h:
+        (JSC::jsString):
+
 2016-09-22  Filip Pizlo  <fpizlo@apple.com>
 
         Fences on x86 should be a lot cheaper
index f107b58..d21916a 100644 (file)
@@ -52,6 +52,7 @@
 #include "JSMap.h"
 #include "JSSet.h"
 #include "ObjectConstructor.h"
+#include "Operations.h"
 #include "RegExpObject.h"
 #include "Repatch.h"
 #include "ScopedArguments.h"
@@ -1500,28 +1501,16 @@ JSCell* JIT_OPERATION operationMakeRope2(ExecState* exec, JSString* left, JSStri
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
-    if (sumOverflows<int32_t>(left->length(), right->length())) {
-        throwOutOfMemoryError(exec, scope);
-        return nullptr;
-    }
 
-    return JSRopeString::create(vm, left, right);
+    return jsString(exec, left, right);
 }
 
 JSCell* JIT_OPERATION operationMakeRope3(ExecState* exec, JSString* a, JSString* b, JSString* c)
 {
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
-    auto scope = DECLARE_THROW_SCOPE(vm);
-
-    if (sumOverflows<int32_t>(a->length(), b->length(), c->length())) {
-        throwOutOfMemoryError(exec, scope);
-        return nullptr;
-    }
 
-    return JSRopeString::create(vm, a, b, c);
+    return jsString(exec, a, b, c);
 }
 
 JSCell* JIT_OPERATION operationStrCat2(ExecState* exec, EncodedJSValue a, EncodedJSValue b)
@@ -1535,12 +1524,8 @@ JSCell* JIT_OPERATION operationStrCat2(ExecState* exec, EncodedJSValue a, Encode
     JSString* str2 = JSValue::decode(b).toString(exec);
     ASSERT(!scope.exception());
 
-    if (sumOverflows<int32_t>(str1->length(), str2->length())) {
-        throwOutOfMemoryError(exec, scope);
-        return nullptr;
-    }
-
-    return JSRopeString::create(vm, str1, str2);
+    scope.release();
+    return jsString(exec, str1, str2);
 }
     
 JSCell* JIT_OPERATION operationStrCat3(ExecState* exec, EncodedJSValue a, EncodedJSValue b, EncodedJSValue c)
@@ -1556,12 +1541,8 @@ JSCell* JIT_OPERATION operationStrCat3(ExecState* exec, EncodedJSValue a, Encode
     JSString* str3 = JSValue::decode(c).toString(exec);
     ASSERT(!scope.exception());
 
-    if (sumOverflows<int32_t>(str1->length(), str2->length(), str3->length())) {
-        throwOutOfMemoryError(exec, scope);
-        return nullptr;
-    }
-
-    return JSRopeString::create(vm, str1, str2, str3);
+    scope.release();
+    return jsString(exec, str1, str2, str3);
 }
 
 char* JIT_OPERATION operationFindSwitchImmTargetForDouble(
index fe22d97..7bc14f0 100644 (file)
@@ -41,6 +41,7 @@
 #include "Lookup.h"
 #include "ObjectConstructor.h"
 #include "ObjectPrototype.h"
+#include "Operations.h"
 #include "StringRecursionChecker.h"
 #include <algorithm>
 #include <wtf/Assertions.h>
@@ -546,7 +547,9 @@ static JSValue slowJoin(ExecState& exec, JSObject* thisObject, JSString* separat
 
         // a. Let S be the String value produced by concatenating R and sep.
         // d. Let R be a String value produced by concatenating S and next.
-        r = JSRopeString::create(vm, r, separator, next);
+        r = jsString(&exec, r, separator, next);
+        if (UNLIKELY(scope.exception()))
+            return JSValue();
     }
     // 10. Return R.
     return r;
index 13c5545..724a496 100644 (file)
@@ -219,7 +219,7 @@ private:
     String& string() { ASSERT(!isRope()); return m_value; }
     StringView unsafeView(ExecState&) const;
 
-    friend JSValue jsString(ExecState*, JSString*, JSString*);
+    friend JSString* jsString(ExecState*, JSString*, JSString*);
     friend JSString* jsSubstring(ExecState*, JSString*, unsigned offset, unsigned length);
 };
 
@@ -277,6 +277,7 @@ private:
     void finishCreation(VM& vm, JSString* s1, JSString* s2)
     {
         Base::finishCreation(vm);
+        ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length()));
         m_length = s1->length() + s2->length();
         setIs8Bit(s1->is8Bit() && s2->is8Bit());
         setIsSubstring(false);
@@ -288,6 +289,7 @@ private:
     void finishCreation(VM& vm, JSString* s1, JSString* s2, JSString* s3)
     {
         Base::finishCreation(vm);
+        ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length(), s3->length()));
         m_length = s1->length() + s2->length() + s3->length();
         setIs8Bit(s1->is8Bit() && s2->is8Bit() &&  s3->is8Bit());
         setIsSubstring(false);
@@ -357,19 +359,6 @@ private:
     }
 
 public:
-    static JSString* create(VM& vm, JSString* s1, JSString* s2)
-    {
-        JSRopeString* newString = new (NotNull, allocateCell<JSRopeString>(vm.heap)) JSRopeString(vm);
-        newString->finishCreation(vm, s1, s2);
-        return newString;
-    }
-    static JSString* create(VM& vm, JSString* s1, JSString* s2, JSString* s3)
-    {
-        JSRopeString* newString = new (NotNull, allocateCell<JSRopeString>(vm.heap)) JSRopeString(vm);
-        newString->finishCreation(vm, s1, s2, s3);
-        return newString;
-    }
-
     static JSString* create(VM& vm, ExecState* exec, JSString* base, unsigned offset, unsigned length)
     {
         JSRopeString* newString = new (NotNull, allocateCell<JSRopeString>(vm.heap)) JSRopeString(vm);
@@ -391,6 +380,19 @@ public:
     static const unsigned s_maxInternalRopeLength = 3;
 
 private:
+    static JSString* create(VM& vm, JSString* s1, JSString* s2)
+    {
+        JSRopeString* newString = new (NotNull, allocateCell<JSRopeString>(vm.heap)) JSRopeString(vm);
+        newString->finishCreation(vm, s1, s2);
+        return newString;
+    }
+    static JSString* create(VM& vm, JSString* s1, JSString* s2, JSString* s3)
+    {
+        JSRopeString* newString = new (NotNull, allocateCell<JSRopeString>(vm.heap)) JSRopeString(vm);
+        newString->finishCreation(vm, s1, s2, s3);
+        return newString;
+    }
+
     friend JSValue jsStringFromRegisterArray(ExecState*, Register*, unsigned);
     friend JSValue jsStringFromArguments(ExecState*, JSValue);
 
@@ -449,6 +451,11 @@ private:
         uintptr_t number;
         WriteBarrierBase<JSString> string;
     } u[s_maxInternalRopeLength];
+
+
+    friend JSString* jsString(ExecState*, JSString*, JSString*);
+    friend JSString* jsString(ExecState*, JSString*, JSString*, JSString*);
+    friend JSString* jsString(ExecState*, const String&, const String&, const String&);
 };
 
 class JSString::SafeView {
index 34e301f..a954ff5 100644 (file)
@@ -37,7 +37,7 @@ bool jsIsObjectTypeOrNull(CallFrame*, JSValue);
 bool jsIsFunctionType(JSValue);
 size_t normalizePrototypeChain(CallFrame*, Structure*);
 
-ALWAYS_INLINE JSValue jsString(ExecState* exec, JSString* s1, JSString* s2)
+ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
 {
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
@@ -48,13 +48,43 @@ ALWAYS_INLINE JSValue jsString(ExecState* exec, JSString* s1, JSString* s2)
     int32_t length2 = s2->length();
     if (!length2)
         return s1;
-    if (sumOverflows<int32_t>(length1, length2))
-        return throwOutOfMemoryError(exec, scope);
+    if (sumOverflows<int32_t>(length1, length2)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
 
     return JSRopeString::create(vm, s1, s2);
 }
 
-ALWAYS_INLINE JSValue jsString(ExecState* exec, const String& u1, const String& u2, const String& u3)
+ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JSString* s3)
+{
+    VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    int32_t length1 = s1->length();
+    if (!length1) {
+        scope.release();
+        return jsString(exec, s2, s3);
+    }
+    int32_t length2 = s2->length();
+    if (!length2) {
+        scope.release();
+        return jsString(exec, s1, s3);
+    }
+    int32_t length3 = s3->length();
+    if (!length3) {
+        scope.release();
+        return jsString(exec, s1, s2);
+    }
+
+    if (sumOverflows<int32_t>(length1, length2, length3)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
+    return JSRopeString::create(vm, s1, s2, s3);
+}
+
+ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String& u2, const String& u3)
 {
     VM* vm = &exec->vm();
     auto scope = DECLARE_THROW_SCOPE(*vm);
@@ -63,20 +93,30 @@ ALWAYS_INLINE JSValue jsString(ExecState* exec, const String& u1, const String&
     int32_t length2 = u2.length();
     int32_t length3 = u3.length();
     
-    if (length1 < 0 || length2 < 0 || length3 < 0)
-        return throwOutOfMemoryError(exec, scope);
+    if (length1 < 0 || length2 < 0 || length3 < 0) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
     
-    if (!length1)
+    if (!length1) {
+        scope.release();
         return jsString(exec, jsString(vm, u2), jsString(vm, u3));
-    if (!length2)
+    }
+    if (!length2) {
+        scope.release();
         return jsString(exec, jsString(vm, u1), jsString(vm, u3));
-    if (!length3)
+    }
+    if (!length3) {
+        scope.release();
         return jsString(exec, jsString(vm, u1), jsString(vm, u2));
+    }
 
-    if (sumOverflows<int32_t>(length1, length2, length3))
-        return throwOutOfMemoryError(exec, scope);
+    if (sumOverflows<int32_t>(length1, length2, length3)) {
+        throwOutOfMemoryError(exec, scope);
+        return nullptr;
+    }
 
-    return JSRopeString::create(exec->vm(), jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
+    return JSRopeString::create(*vm, jsString(vm, u1), jsString(vm, u2), jsString(vm, u3));
 }
 
 ALWAYS_INLINE JSValue jsStringFromRegisterArray(ExecState* exec, Register* strings, unsigned count)