Fix max length check in ArrayPrototype.js' concatSlowPath().
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Feb 2017 01:54:24 +0000 (01:54 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 10 Feb 2017 01:54:24 +0000 (01:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167270
<rdar://problem/30128133>

Patch by Mark Lam <mark.lam@apple.com> on 2017-02-09
Reviewed by Filip Pizlo.

JSTests:

* stress/array-prototype-concat-of-long-spliced-arrays.js: Added.
* stress/array-prototype-concat-of-long-spliced-arrays2.js: Added.

Source/JavaScriptCore:

1. Fixed concatSlowPath() to ensure that the result array length does not exceed
   @MAX_ARRAY_INDEX.  The old code was checking against @MAX_SAFE_INTEGER in some
   cases, but this is overly permissive.

2. Changed concatSlowPath() to throw a RangeError instead of a TypeError to be
   consistent with the C++ runtime functions in JSArray.cpp.

3. Changed the RangeError message in concatSlowPath() and JSArray.cpp to "Length
   exceeded the maximum array length" when the error is that the result length
   exceeds MAX_ARRAY_INDEX.  We do this for 2 reasons:
   a. "Length exceeded the maximum array length" is more informative than
      "Invalid array length".
   b. We want to use the same string consistently for the same error.

   There are still 2 places in JSArray.cpp that still throws a RangeError with
   message "Invalid array length".  In those cases, the error is not necessarily
   due to the result length exceeding MAX_ARRAY_INDEX, but is due to attempting to
   set a length value that is not an integer that fits in MAX_ARRAY_INDEX e.g.
   an attempt to set a fractional length value.  Hence, "Invalid array length" is
   appropriate for those cases.

4. Fixed JSArray::appendMemcpy() to handle overflows when computing the result
   array length.

* builtins/ArrayPrototype.js:
(concatSlowPath):
* bytecode/BytecodeIntrinsicRegistry.cpp:
(JSC::BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry):
* bytecode/BytecodeIntrinsicRegistry.h:
* runtime/ArrayPrototype.cpp:
(JSC::concatAppendOne):
(JSC::arrayProtoPrivateFuncAppendMemcpy):
* runtime/JSArray.cpp:
(JSC::JSArray::appendMemcpy):
(JSC::JSArray::push):

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

JSTests/ChangeLog
JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js [new file with mode: 0644]
JSTests/stress/array-prototype-concat-of-long-spliced-arrays2.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/builtins/ArrayPrototype.js
Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cpp
Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h
Source/JavaScriptCore/runtime/ArrayPrototype.cpp
Source/JavaScriptCore/runtime/JSArray.cpp

index 8a0a5f5..d3f6095 100644 (file)
@@ -1,3 +1,14 @@
+2017-02-09  Mark Lam  <mark.lam@apple.com>
+
+        Fix max length check in ArrayPrototype.js' concatSlowPath().
+        https://bugs.webkit.org/show_bug.cgi?id=167270
+        <rdar://problem/30128133>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/array-prototype-concat-of-long-spliced-arrays.js: Added.
+        * stress/array-prototype-concat-of-long-spliced-arrays2.js: Added.
+
 2017-02-09  Keith Miller  <keith_miller@apple.com>
 
         We should not allow Function.caller to be used on native functions
diff --git a/JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js b/JSTests/stress/array-prototype-concat-of-long-spliced-arrays.js
new file mode 100644 (file)
index 0000000..8658a92
--- /dev/null
@@ -0,0 +1,36 @@
+function shouldEqual(actual, expected) {
+    if (actual != expected) {
+        throw "ERROR: expect " + expected + ", actual " + actual;
+    }
+}
+
+function test() {
+    var exception;
+    try {
+        var a = [];
+        a.length = 0xffffff00;
+
+        var b = a.splice(0, 0x100000); // Undecided array
+
+        var args = [];
+        args.length = 4094;
+        args.fill(b);
+
+        var q = [];
+        q.length = 0x1000;
+        q.fill(7);
+
+        var c = a.splice(0, 0xfffef); //Shorter undecided array
+
+        args[4094] = c;
+        args[4095] = q;
+
+        b.concat.apply(b, args);
+    } catch (e) {
+        exception = e;
+    }
+
+    shouldEqual(exception, "RangeError: Length exceeded the maximum array length");
+}
+
+test();
diff --git a/JSTests/stress/array-prototype-concat-of-long-spliced-arrays2.js b/JSTests/stress/array-prototype-concat-of-long-spliced-arrays2.js
new file mode 100644 (file)
index 0000000..49e4c8d
--- /dev/null
@@ -0,0 +1,26 @@
+function shouldEqual(actual, expected) {
+    if (actual != expected) {
+        throw "ERROR: expect " + expected + ", actual " + actual;
+    }
+}
+
+function test() {
+    var exception;
+    try {
+        var a = [];
+        a.length = 0xffffff00;
+
+        var b = a.splice(0, 0x100000); // Undecided array
+
+        var args = [];
+        args.length = 4096;
+        args.fill(b);
+
+        b.concat.apply(b, args);
+    } catch (e) {
+        exception = e;
+    }
+    shouldEqual(exception, "RangeError: Length exceeded the maximum array length");
+}
+
+test();
index 11039d2..4e8caf6 100644 (file)
@@ -1,5 +1,49 @@
 2017-02-09  Mark Lam  <mark.lam@apple.com>
 
+        Fix max length check in ArrayPrototype.js' concatSlowPath().
+        https://bugs.webkit.org/show_bug.cgi?id=167270
+        <rdar://problem/30128133>
+
+        Reviewed by Filip Pizlo.
+
+        1. Fixed concatSlowPath() to ensure that the result array length does not exceed
+           @MAX_ARRAY_INDEX.  The old code was checking against @MAX_SAFE_INTEGER in some
+           cases, but this is overly permissive.
+
+        2. Changed concatSlowPath() to throw a RangeError instead of a TypeError to be
+           consistent with the C++ runtime functions in JSArray.cpp.
+
+        3. Changed the RangeError message in concatSlowPath() and JSArray.cpp to "Length
+           exceeded the maximum array length" when the error is that the result length
+           exceeds MAX_ARRAY_INDEX.  We do this for 2 reasons:
+           a. "Length exceeded the maximum array length" is more informative than
+              "Invalid array length".
+           b. We want to use the same string consistently for the same error.
+
+           There are still 2 places in JSArray.cpp that still throws a RangeError with
+           message "Invalid array length".  In those cases, the error is not necessarily
+           due to the result length exceeding MAX_ARRAY_INDEX, but is due to attempting to
+           set a length value that is not an integer that fits in MAX_ARRAY_INDEX e.g.
+           an attempt to set a fractional length value.  Hence, "Invalid array length" is
+           appropriate for those cases.
+
+        4. Fixed JSArray::appendMemcpy() to handle overflows when computing the result
+           array length.
+
+        * builtins/ArrayPrototype.js:
+        (concatSlowPath):
+        * bytecode/BytecodeIntrinsicRegistry.cpp:
+        (JSC::BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry):
+        * bytecode/BytecodeIntrinsicRegistry.h:
+        * runtime/ArrayPrototype.cpp:
+        (JSC::concatAppendOne):
+        (JSC::arrayProtoPrivateFuncAppendMemcpy):
+        * runtime/JSArray.cpp:
+        (JSC::JSArray::appendMemcpy):
+        (JSC::JSArray::push):
+
+2017-02-09  Mark Lam  <mark.lam@apple.com>
+
         Constructed object's global object should be the global object of the constructor.
         https://bugs.webkit.org/show_bug.cgi?id=167121
         <rdar://problem/30054759>
index e780db3..b9f8355 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2014-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -686,12 +686,12 @@ function concatSlowPath()
         let spreadable = @isObject(currentElement) && currentElement.@isConcatSpreadableSymbol;
         if ((spreadable === @undefined && @isArray(currentElement)) || spreadable) {
             let length = @toLength(currentElement.length);
+            if (length + resultIndex > @MAX_ARRAY_INDEX)
+                @throwRangeError("Length exceeded the maximum array length");
             if (resultIsArray && @isJSArray(currentElement)) {
                 @appendMemcpy(result, currentElement, resultIndex);
                 resultIndex += length;
             } else {
-                if (length + resultIndex > @MAX_SAFE_INTEGER)
-                    @throwTypeError("length exceeded the maximum safe integer");
                 for (var i = 0; i < length; i++) {
                     if (i in currentElement)
                         @putByValDirect(result, resultIndex, currentElement[i]);
@@ -699,8 +699,8 @@ function concatSlowPath()
                 }
             }
         } else {
-            if (resultIndex >= @MAX_SAFE_INTEGER)
-                @throwTypeError("length exceeded the maximum safe integer");
+            if (resultIndex >= @MAX_ARRAY_INDEX)
+                @throwRangeError("Length exceeded the maximum array length");
             @putByValDirect(result, resultIndex++, currentElement);
         }
         currentElement = arguments[argIndex];
index 7406b52..00c9c01 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>.
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -53,6 +53,7 @@ BytecodeIntrinsicRegistry::BytecodeIntrinsicRegistry(VM& vm)
     m_iterationKindKey.set(m_vm, jsNumber(IterateKey));
     m_iterationKindValue.set(m_vm, jsNumber(IterateValue));
     m_iterationKindKeyValue.set(m_vm, jsNumber(IterateKeyValue));
+    m_MAX_ARRAY_INDEX.set(m_vm, jsNumber(MAX_ARRAY_INDEX));
     m_MAX_STRING_LENGTH.set(m_vm, jsNumber(JSString::MaxLength));
     m_MAX_SAFE_INTEGER.set(m_vm, jsDoubleNumber(maxSafeInteger()));
     m_ModuleFetch.set(m_vm, jsNumber(static_cast<unsigned>(JSModuleLoader::Status::Fetch)));
index 19e5d7c..0259bc6 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2015 Yusuke Suzuki <utatane.tea@gmail.com>.
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -64,6 +64,7 @@ class Identifier;
     macro(iterationKindKey) \
     macro(iterationKindValue) \
     macro(iterationKindKeyValue) \
+    macro(MAX_ARRAY_INDEX) \
     macro(MAX_STRING_LENGTH) \
     macro(MAX_SAFE_INTEGER) \
     macro(ModuleFetch) \
index c8ad4d1..8376fd3 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2003, 2007-2009, 2011, 2013, 2015-2016 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003, 2007-2009, 2011, 2013, 2015-2017 Apple Inc. All rights reserved.
  *  Copyright (C) 2003 Peter Kelly (pmk@post.com)
  *  Copyright (C) 2006 Alexey Proskuryakov (ap@nypop.com)
  *
@@ -1216,8 +1216,10 @@ static EncodedJSValue concatAppendOne(ExecState* exec, VM& vm, JSArray* first, J
         return JSValue::encode(throwOutOfMemoryError(exec, scope));
 
     bool success = result->appendMemcpy(exec, vm, 0, first);
-    RETURN_IF_EXCEPTION(scope, encodedJSValue());
+    ASSERT(!success || !scope.exception());
     if (!success) {
+        RETURN_IF_EXCEPTION(scope, encodedJSValue());
+
         bool success = moveElements(exec, vm, result, 0, first, firstArraySize);
         ASSERT(!scope.exception() == success);
         if (UNLIKELY(!success))
@@ -1311,14 +1313,18 @@ EncodedJSValue JSC_HOST_CALL arrayProtoPrivateFuncAppendMemcpy(ExecState* exec)
     ASSERT(exec->argumentCount() == 3);
 
     VM& vm = exec->vm();
+    auto scope = DECLARE_THROW_SCOPE(vm);
     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());
-    if (!resultArray->appendMemcpy(exec, vm, startIndex, otherArray))
+    bool success = resultArray->appendMemcpy(exec, vm, startIndex, otherArray);
+    ASSERT(!success || !scope.exception());
+    if (!success) {
+        RETURN_IF_EXCEPTION(scope, encodedJSValue());
         moveElements(exec, vm, resultArray, startIndex, otherArray, otherArray->length());
-
+    }
     return JSValue::encode(jsUndefined());
 }
 
index 00ff53b..7eebdc2 100644 (file)
@@ -40,6 +40,8 @@ using namespace WTF;
 
 namespace JSC {
 
+static const char* const LengthExceededTheMaximumArrayLengthError = "Length exceeded the maximum array length";
+
 STATIC_ASSERT_IS_TRIVIALLY_DESTRUCTIBLE(JSArray);
 
 const ClassInfo JSArray::s_info = {"Array", &JSNonFinalObject::s_info, 0, CREATE_METHOD_TABLE(JSArray)};
@@ -495,7 +497,15 @@ bool JSArray::appendMemcpy(ExecState* exec, VM& vm, unsigned startIndex, JSC::JS
         return false;
 
     unsigned otherLength = otherArray->length();
-    unsigned newLength = startIndex + otherLength;
+    Checked<unsigned, RecordOverflow> checkedNewLength = startIndex;
+    checkedNewLength += otherLength;
+
+    unsigned newLength;
+    if (checkedNewLength.safeGet(newLength) == CheckedState::DidOverflow) {
+        throwException(exec, scope, createRangeError(exec, ASCIILiteral(LengthExceededTheMaximumArrayLengthError)));
+        return false;
+    }
+
     if (newLength >= MIN_SPARSE_ARRAY_INDEX)
         return false;
 
@@ -724,7 +734,7 @@ void JSArray::push(ExecState* exec, JSValue value)
         if (UNLIKELY(length > MAX_ARRAY_INDEX)) {
             methodTable(vm)->putByIndex(this, exec, length, value, true);
             if (!scope.exception())
-                throwException(exec, scope, createRangeError(exec, ASCIILiteral("Invalid array length")));
+                throwException(exec, scope, createRangeError(exec, ASCIILiteral(LengthExceededTheMaximumArrayLengthError)));
             return;
         }
 
@@ -745,7 +755,7 @@ void JSArray::push(ExecState* exec, JSValue value)
         if (UNLIKELY(length > MAX_ARRAY_INDEX)) {
             methodTable(vm)->putByIndex(this, exec, length, value, true);
             if (!scope.exception())
-                throwException(exec, scope, createRangeError(exec, ASCIILiteral("Invalid array length")));
+                throwException(exec, scope, createRangeError(exec, ASCIILiteral(LengthExceededTheMaximumArrayLengthError)));
             return;
         }
 
@@ -780,7 +790,7 @@ void JSArray::push(ExecState* exec, JSValue value)
         if (UNLIKELY(length > MAX_ARRAY_INDEX)) {
             methodTable(vm)->putByIndex(this, exec, length, value, true);
             if (!scope.exception())
-                throwException(exec, scope, createRangeError(exec, ASCIILiteral("Invalid array length")));
+                throwException(exec, scope, createRangeError(exec, ASCIILiteral(LengthExceededTheMaximumArrayLengthError)));
             return;
         }
 
@@ -819,7 +829,7 @@ void JSArray::push(ExecState* exec, JSValue value)
             methodTable(vm)->putByIndex(this, exec, storage->length(), value, true);
             // Per ES5.1 15.4.4.7 step 6 & 15.4.5.1 step 3.d.
             if (!scope.exception())
-                throwException(exec, scope, createRangeError(exec, ASCIILiteral("Invalid array length")));
+                throwException(exec, scope, createRangeError(exec, ASCIILiteral(LengthExceededTheMaximumArrayLengthError)));
             return;
         }