[builtins] Fix Array.p.join length overflow and invalid string length handling

- Fixes and simplify allocating the temporary fixed array for ToString-ed elements.
  - When the array size is greater than representable by an intptr, it overflowed into a negative value causing a non-negative assert to fail.
  - Simplify fallback behavior by always allocating a conservatively sized temporary fixed array. Previously, if the array had dictionary elements, the temporary fixed array was sized based on %GetNumberDictionaryNumberOfElements() and then resized when entering the fallback.

- Fixes related invalid string length handling. When the running total of the resulting string length overflowed or exceeded String::kMaxLength, a RangeError is thrown. Previously, this thrown RangeError bypassed JoinStackPop and left the receiver on the stack.

Bug: chromium:897404
Change-Id: I157b71ef04ab06125a5b1c3454e5ed3713bdb591
Reviewed-on: https://chromium-review.googlesource.com/c/1293070
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56907}

src/builtins/array-join.tq

@@ -10,15 +10,9 @@ module array {
   extern macro CallJSArrayArrayJoinConcatToSequentialString(
       FixedArray, intptr, String, String): String;
 
-  extern macro CallLoadJoinElement(implicit context: Context)(
-      LoadJoinElementFn, JSReceiver, Number): Object
-      labels IfException(Object);
-
-  extern macro CallConvertToLocaleString(implicit context: Context)(
-      Object, Object, Object): String
-      labels IfException(Object);
-
-  extern macro CallToString(implicit context: Context)(Object): String
+  extern macro CallArrayJoin(
+      Context, constexpr bool, JSReceiver, String, Number, Object, Object):
+      String
       labels IfException(Object);
 
   builtin LoadJoinElement<T: type>(
@@ -148,11 +142,11 @@ module array {
     isOneByte: bool;
   }
 
-  macro BufferInit(estimatedNonHoleyElements: uintptr, sep: String): Buffer {
-    const bufferSize: intptr = Signed(estimatedNonHoleyElements + 1);
-    const cappedBufferSize: intptr = bufferSize > kFixedArrayMaxLength ?
+  macro BufferInit(len: uintptr, sep: String): Buffer {
+    const cappedBufferSize: intptr = len > kFixedArrayMaxLength ?
         FromConstexpr<intptr>(kFixedArrayMaxLength) :
-        bufferSize;
+        Signed(len);
+    assert(cappedBufferSize > 0);
     const fixedArray: FixedArray = AllocateZeroedFixedArray(cappedBufferSize);
     const isOneByte: bool = HasOnlyOneByteChars(sep.instanceType);
     return Buffer{fixedArray, 0, 0, isOneByte};
@@ -220,76 +214,62 @@ module array {
   macro ArrayJoinImpl(
       context: Context, receiver: JSReceiver, sep: String, lengthNumber: Number,
       useToLocaleString: constexpr bool, locales: Object, options: Object,
-      estimatedNonHoleyElements: uintptr,
-      initialLoadJoinElement: LoadJoinElementFn): String
-      labels IfException(Object) {
+      initialLoadJoinElement: LoadJoinElementFn): String {
     const initialMap: Map = receiver.map;
     const len: uintptr = Convert<uintptr>(lengthNumber);
     const separatorLength: intptr = sep.length;
     let nofSeparators: intptr = 0;
     let loadJoinElements: LoadJoinElementFn = initialLoadJoinElement;
-    let buffer: Buffer = BufferInit(estimatedNonHoleyElements, sep);
-
-    if (estimatedNonHoleyElements != 0) {
-      // 6. Let k be 0.
-      let k: uintptr = 0;
-
-      // 7. Repeat, while k < len
-      while (k < len) {
-        if (k > 0) {
-          // a. If k > 0, let R be the string-concatenation of R and sep.
-          nofSeparators = nofSeparators + 1;
-
-          // Verify the current LoadJoinElement specialization can safely be
-          // used. Otherwise, fall back to generic element access (see
-          // LoadJoinElement<GenericElementsAccessor>).
-          if (loadJoinElements != LoadJoinElement<GenericElementsAccessor>&&
-              CannotUseSameArrayAccessor(initialMap, lengthNumber, receiver))
-            deferred {
-              loadJoinElements = LoadJoinElement<GenericElementsAccessor>;
-
-              // Join the current buffer into a single string and add it to a
-              // new buffer that the fall back will continue with.
-              const temp: String = BufferJoin(buffer, sep);
-              buffer = BufferInit((len - k + 1), sep);
-              buffer = BufferAdd(buffer, temp, 0, separatorLength);
-            }
-        }
+    let buffer: Buffer = BufferInit(len, sep);
+
+    // 6. Let k be 0.
+    let k: uintptr = 0;
+
+    // 7. Repeat, while k < len
+    while (k < len) {
+      if (k > 0) {
+        // a. If k > 0, let R be the string-concatenation of R and sep.
+        nofSeparators = nofSeparators + 1;
+
+        // Verify the current LoadJoinElement specialization can safely be
+        // used. Otherwise, fall back to generic element access (see
+        // LoadJoinElement<GenericElementsAccessor>).
+        if (loadJoinElements != LoadJoinElement<GenericElementsAccessor>&&
+            CannotUseSameArrayAccessor(initialMap, lengthNumber, receiver))
+          deferred {
+            loadJoinElements = LoadJoinElement<GenericElementsAccessor>;
+          }
+      }
 
-        // b. Let element be ? Get(O, ! ToString(k)).
-        const element: Object = CallLoadJoinElement(
-            loadJoinElements, receiver, Convert<Number>(k++))
-            otherwise IfException;
-
-        // c. If element is undefined or null, let next be the empty String;
-        //    otherwise, let next be ? ToString(element).
-        let next: String;
-        if constexpr (useToLocaleString) {
-          next = CallConvertToLocaleString(element, locales, options)
-              otherwise IfException;
-          if (next == kEmptyString) continue;
-        } else {
-          typeswitch (element) {
-            case (str: String): {
-              if (str == kEmptyString) continue;
-              next = str;
-            }
-            case (num: Number): {
-              next = NumberToString(num);
-            }
-            case (obj: HeapObject): {
-              if (IsNullOrUndefined(obj)) continue;
-              next = CallToString(obj) otherwise IfException;
-            }
+      // b. Let element be ? Get(O, ! ToString(k)).
+      const element: Object =
+          loadJoinElements(context, receiver, Convert<Number>(k++));
+
+      // c. If element is undefined or null, let next be the empty String;
+      //    otherwise, let next be ? ToString(element).
+      let next: String;
+      if constexpr (useToLocaleString) {
+        next = ConvertToLocaleString(context, element, locales, options);
+        if (next == kEmptyString) continue;
+      } else {
+        typeswitch (element) {
+          case (str: String): {
+            if (str == kEmptyString) continue;
+            next = str;
+          }
+          case (num: Number): {
+            next = NumberToString(num);
+          }
+          case (obj: HeapObject): {
+            if (IsNullOrUndefined(obj)) continue;
+            next = ToString(context, obj);
           }
         }
-
-        // d. Set R to the string-concatenation of R and next.
-        buffer = BufferAdd(buffer, next, nofSeparators, separatorLength);
-        nofSeparators = 0;
       }
-    } else {
-      nofSeparators = Signed(len - 1);
+
+      // d. Set R to the string-concatenation of R and next.
+      buffer = BufferAdd(buffer, next, nofSeparators, separatorLength);
+      nofSeparators = 0;
     }
 
     // Add any separators at the end.
@@ -301,8 +281,7 @@ module array {
 
   macro ArrayJoin(implicit context: Context)(
       useToLocaleString: constexpr bool, receiver: JSReceiver, sep: String,
-      lenNumber: Number, locales: Object, options: Object): Object
-      labels IfException(Object) {
+      lenNumber: Number, locales: Object, options: Object): Object {
     const map: Map = receiver.map;
     const kind: ElementsKind = map.elements_kind;
     const len: uintptr = Convert<uintptr>(lenNumber);
@@ -310,7 +289,6 @@ module array {
     // Estimated number of elements that are not holes. This is conservatively
     // defaulted to `len`. When the receiver has dictionary elements, a better
     // estimate can be determined through GetNumberDictionaryNumberOfElements.
-    let estimatedNonHoleyElements: uintptr = len;
     let loadJoinElements: LoadJoinElementFn;
 
     try {
@@ -323,14 +301,26 @@ module array {
         loadJoinElements = LoadJoinElement<FastSmiOrObjectElements>;
       } else if (IsElementsKindLessThanOrEqual(kind, HOLEY_DOUBLE_ELEMENTS)) {
         loadJoinElements = LoadJoinElement<FastDoubleElements>;
-      } else if (kind == DICTIONARY_ELEMENTS) {
-        const dict: NumberDictionary =
-            UnsafeCast<NumberDictionary>(array.elements);
-        estimatedNonHoleyElements =
-            Unsigned(Convert<intptr>(GetNumberDictionaryNumberOfElements(dict)))
-            << 1;
-        loadJoinElements = LoadJoinElement<DictionaryElements>;
-      } else {
+      } else if (kind == DICTIONARY_ELEMENTS)
+        deferred {
+          const dict: NumberDictionary =
+              UnsafeCast<NumberDictionary>(array.elements);
+          const nofElements: Smi = GetNumberDictionaryNumberOfElements(dict);
+          if (nofElements == 0) {
+            if (sep == kEmptyString) return kEmptyString;
+            try {
+              const nofSeparators: Smi =
+                  Cast<Smi>(lenNumber - 1) otherwise IfNotSmi;
+              return StringRepeat(context, sep, nofSeparators);
+            }
+            label IfNotSmi {
+              ThrowRangeError(context, kInvalidStringLength);
+            }
+          } else {
+            loadJoinElements = LoadJoinElement<DictionaryElements>;
+          }
+        }
+      else {
         goto IfSlowPath;
       }
     }
@@ -339,8 +329,19 @@ module array {
     }
     return ArrayJoinImpl(
         context, receiver, sep, lenNumber, useToLocaleString, locales, options,
-        estimatedNonHoleyElements, loadJoinElements)
-        otherwise IfException;
+        loadJoinElements);
+  }
+
+  builtin ArrayJoinWithToLocaleString(
+      context: Context, receiver: JSReceiver, sep: String, len: Number,
+      locales: Object, options: Object): Object {
+    return ArrayJoin(true, receiver, sep, len, locales, options);
+  }
+
+  builtin ArrayJoinWithoutToLocaleString(
+      context: Context, receiver: JSReceiver, sep: String, len: Number,
+      locales: Object, options: Object): Object {
+    return ArrayJoin(false, receiver, sep, len, locales, options);
   }
 
   // The Join Stack detects cyclical calls to Array Join builtins
@@ -472,8 +473,8 @@ module array {
 
       JoinStackPushInline(o) otherwise IfReturnEmpty;
 
-      const result: Object =
-          ArrayJoin(useToLocaleString, o, sep, len, locales, options)
+      const result: Object = CallArrayJoin(
+          context, useToLocaleString, o, sep, len, locales, options)
           otherwise IfException;
 
       JoinStackPopInline(o);

src/builtins/base.tq

@@ -1168,3 +1168,4 @@ extern macro TryIntPtrAdd(intptr, intptr): intptr
     labels IfOverflow;
 
 extern builtin ObjectToString(Context, Object): Object;
+extern builtin StringRepeat(Context, String, Number): String;

src/builtins/builtins-array-gen.h

@@ -91,43 +91,16 @@ class ArrayBuiltinsAssembler : public BaseBuiltinsFromDSLAssembler {
 
   // Temporary Torque support for Array.prototype.join().
   // TODO(pwong): Remove this when Torque supports exception handlers.
-  TNode<Object> CallLoadJoinElement(TNode<Context> context,
-                                    TNode<Code> loadJoinElement,
-                                    TNode<JSReceiver> receiver, TNode<Number> k,
-                                    Label* if_exception,
-                                    TVariable<Object>* var_exception) {
-    // Calling a specialization of LoadJoinElement (see array-join.tq), requires
-    // a descriptor.  We arbitrarily use one of specialization's descriptor, as
-    // all specializations share the same interface.
-    TNode<Object> result = CallStub(
-        Builtins::CallableFor(isolate(),
-                              Builtins::kLoadJoinElement20ATDictionaryElements)
-            .descriptor(),
-        loadJoinElement, context, receiver, k);
-    GotoIfException(result, if_exception, var_exception);
-    return result;
-  }
-
-  // Temporary Torque support for Array.prototype.join().
-  // TODO(pwong): Remove this when Torque supports exception handlers.
-  TNode<String> CallConvertToLocaleString(TNode<Context> context,
-                                          TNode<Object> element,
-                                          TNode<Object> locales,
-                                          TNode<Object> options,
-                                          Label* if_exception,
-                                          TVariable<Object>* var_exception) {
-    TNode<Object> result = CallBuiltin(Builtins::kConvertToLocaleString,
-                                       context, element, locales, options);
-    GotoIfException(result, if_exception, var_exception);
-    return CAST(result);
-  }
-
-  // Temporary Torque support for Array.prototype.join().
-  // TODO(pwong): Remove this when Torque supports exception handlers.
-  TNode<String> CallToString(TNode<Context> context, TNode<Object> obj,
-                             Label* if_exception,
-                             TVariable<Object>* var_exception) {
-    TNode<Object> result = CallBuiltin(Builtins::kToString, context, obj);
+  TNode<String> CallArrayJoin(TNode<Context> context, bool use_to_locale_string,
+                              TNode<JSReceiver> receiver, TNode<String> sep,
+                              TNode<Number> len, TNode<Object> locales,
+                              TNode<Object> options, Label* if_exception,
+                              TVariable<Object>* var_exception) {
+    Builtins::Name builtin = use_to_locale_string
+                                 ? Builtins::kArrayJoinWithToLocaleString
+                                 : Builtins::kArrayJoinWithoutToLocaleString;
+    TNode<Object> result =
+        CallBuiltin(builtin, context, receiver, sep, len, locales, options);
     GotoIfException(result, if_exception, var_exception);
     return CAST(result);
   }

src/builtins/builtins-string-gen.cc

@@ -1257,7 +1257,6 @@ TF_BUILTIN(StringRepeat, StringBuiltinsAssembler) {
   CSA_ASSERT(this, IsString(string));
   CSA_ASSERT(this, Word32BinaryNot(IsEmptyString(string)));
   CSA_ASSERT(this, TaggedIsPositiveSmi(count));
-  CSA_ASSERT(this, SmiLessThanOrEqual(count, SmiConstant(String::kMaxLength)));
 
   // The string is repeated with the following algorithm:
   //   let n = count;

src/torque/csa-generator.cc

@@ -254,7 +254,11 @@ void CSAGenerator::EmitInstruction(
       out_ << ", &" << var_names[i][j];
     }
   }
-  out_ << ");\n";
+  if (return_type->IsStructType()) {
+    out_ << ").Flatten();\n";
+  } else {
+    out_ << ");\n";
+  }
   if (instruction.return_continuation) {
     out_ << "    Goto(&" << BlockName(*instruction.return_continuation);
     for (const std::string& value : *stack) {

test/mjsunit/array-join-invalid-string-length.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+(function DictionaryStringRepeatFastPath() {
+  const a = new Array(%StringMaxLength());
+  assertTrue(%HasDictionaryElements(a));
+  const sep = '12';
+  assertThrows(() => a.join(sep), RangeError);
+
+  // Verifies cycle detection still works properly after thrown error.
+  assertThrows(() => a.join(sep), RangeError);
+
+  a.length = 3;
+  a[0] = 'a';
+  a[1] = 'b';
+  a[2] = 'c';
+  assertSame('a,b,c', a.join());
+})();
+
+(function SeparatorOverflow() {
+  const a = ['a',,,,,'b'];
+
+  const sep = ','.repeat(%StringMaxLength());
+  assertThrows(() => a.join(sep), RangeError);
+
+  // Verifies cycle detection still works properly after thrown error.
+  assertThrows(() => a.join(sep), RangeError);
+  assertSame('a,,,,,b', a.join());
+})();
+
+(function ElementOverflow() {
+  const el = ','.repeat(%StringMaxLength());
+  const a = [el, el, el, el, el];
+
+  assertThrows(() => a.join(), RangeError);
+
+  // Verifies cycle detection still works properly after thrown error.
+  assertThrows(() => a.join(), RangeError);
+  a[0] = 'a';
+  a[1] = 'b';
+  a[2] = 'c';
+  a[3] = 'd';
+  a[4] = 'e';
+  assertSame('a,b,c,d,e', a.join());
+})();
+
+(function ElementSeparatorOverflow() {
+  const el = ','.repeat(%StringMaxLength());
+  const a = [el, el, el, el];
+
+  assertThrows(() => a.join(el), RangeError);
+
+  // Verifies cycle detection still works properly after thrown error.
+  assertThrows(() => a.join(el), RangeError);
+  a[0] = 'a';
+  a[1] = 'b';
+  a[2] = 'c';
+  a[3] = 'd';
+  assertSame('a,b,c,d', a.join());
+})();

test/mjsunit/messages.js

@@ -570,6 +570,10 @@ test(function() {
   "a".repeat(1 << 30);
 }, "Invalid string length", RangeError);
 
+test(function() {
+  new Array(1 << 30).join();
+}, "Invalid string length", RangeError);
+
 // kNormalizationForm
 test(function() {
   "".normalize("ABC");

test/mjsunit/regress/regress-crbug-897404.js

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+function TestError() {}
+
+// Force slow, generic assess path that will always allocate a temporary fixed
+// array.
+String.prototype.__defineGetter__(0, function() { });
+
+const a = new Array(2**32 - 1);
+
+// Force early exit to avoid an unreasonably long test.
+a[0] = {
+  toString() { throw new TestError(); }
+};
+
+// Verify join throws test error and does not fail due to asserts (Negative
+// length fixed array allocation).
+assertThrows(() => a.join(), TestError);