[typedarray] Fix incorrect optimization in IterableToList

Bug: chromium:854066
Change-Id: Icabd9bf5e00868822b9debfb9bbb5d3932726465
Reviewed-on: https://chromium-review.googlesource.com/1105774
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#53840}

src/builtins/builtins-call-gen.cc

@@ -317,8 +317,8 @@ void CallOrConstructBuiltinsAssembler::CallOrConstructWithSpread(
     TNode<Object> iterator_fn =
         GetProperty(context, spread, IteratorSymbolConstant());
     GotoIfNot(TaggedIsCallable(iterator_fn), &if_iterator_fn_not_callable);
-    TNode<JSArray> list = CAST(
-        CallBuiltin(Builtins::kIterableToList, context, spread, iterator_fn));
+    TNode<JSArray> list = CAST(CallBuiltin(Builtins::kIterableToListUnsafe,
+                                           context, spread, iterator_fn));
     var_length = LoadAndUntagToWord32ObjectField(list, JSArray::kLengthOffset);
 
     var_elements = LoadElements(list);

src/builtins/builtins-definitions.h

@@ -1120,6 +1120,7 @@ namespace internal {
                                                                                \
   /* TypedArray */                                                             \
   TFS(IterableToList, kIterable, kIteratorFn)                                  \
+  TFS(IterableToListUnsafe, kIterable, kIteratorFn)                            \
   TFS(TypedArrayInitialize, kHolder, kLength, kElementSize, kInitialize,       \
       kBufferConstructor)                                                      \
   TFS(TypedArrayInitializeWithBuffer, kHolder, kLength, kBuffer, kElementSize, \

src/builtins/builtins-typed-array-gen.cc

@@ -1650,11 +1650,69 @@ TF_BUILTIN(TypedArrayOf, TypedArrayBuiltinsAssembler) {
                  "%TypedArray%.of");
 }
 
+void TypedArrayBuiltinsAssembler::IterableToListSlowPath(
+    TNode<Context> context, TNode<Object> iterable, TNode<Object> iterator_fn,
+    Variable* created_list) {
+  IteratorBuiltinsAssembler iterator_assembler(state());
+
+  // 1. Let iteratorRecord be ? GetIterator(items, method).
+  IteratorRecord iterator_record =
+      iterator_assembler.GetIterator(context, iterable, iterator_fn);
+
+  // 2. Let values be a new empty List.
+  GrowableFixedArray values(state());
+
+  Variable* vars[] = {values.var_array(), values.var_length(),
+                      values.var_capacity()};
+  Label loop_start(this, 3, vars), loop_end(this);
+  Goto(&loop_start);
+  // 3. Let next be true.
+  // 4. Repeat, while next is not false
+  BIND(&loop_start);
+  {
+    //  a. Set next to ? IteratorStep(iteratorRecord).
+    TNode<Object> next = CAST(
+        iterator_assembler.IteratorStep(context, iterator_record, &loop_end));
+    //  b. If next is not false, then
+    //   i. Let nextValue be ? IteratorValue(next).
+    TNode<Object> next_value =
+        CAST(iterator_assembler.IteratorValue(context, next));
+    //   ii. Append nextValue to the end of the List values.
+    values.Push(next_value);
+    Goto(&loop_start);
+  }
+  BIND(&loop_end);
+
+  // 5. Return values.
+  TNode<JSArray> js_array_values = values.ToJSArray(context);
+  created_list->Bind(js_array_values);
+}
+
+// Unlike IterableToListUnsafe, this builtin always returns a new JSArray
+// and is thus safe to use even in the presence of code that may call back
+// into user-JS.
 TF_BUILTIN(IterableToList, TypedArrayBuiltinsAssembler) {
   TNode<Context> context = CAST(Parameter(Descriptor::kContext));
   TNode<Object> iterable = CAST(Parameter(Descriptor::kIterable));
   TNode<Object> iterator_fn = CAST(Parameter(Descriptor::kIteratorFn));
 
+  TVARIABLE(JSArray, created_list);
+  IterableToListSlowPath(context, iterable, iterator_fn, &created_list);
+
+  Return(created_list.value());
+}
+
+// This verison of IterableToList does not follow the spec, in that it returns
+// the input array if the iteration would be unobservable. This means that if
+// input iterable is still available in user-JS, it can be modified and make the
+// following code incorrect and potentially unsafe. We do this as an
+// optimization for spread calls, where the elements are pushed to the stack
+// with no user-JS being run inbetween.
+TF_BUILTIN(IterableToListUnsafe, TypedArrayBuiltinsAssembler) {
+  TNode<Context> context = CAST(Parameter(Descriptor::kContext));
+  TNode<Object> iterable = CAST(Parameter(Descriptor::kIterable));
+  TNode<Object> iterator_fn = CAST(Parameter(Descriptor::kIteratorFn));
+
   Label fast_path(this), slow_path(this), done(this);
 
   TVARIABLE(JSArray, created_list);
@@ -1674,39 +1732,7 @@ TF_BUILTIN(IterableToList, TypedArrayBuiltinsAssembler) {
 
   BIND(&slow_path);
   {
-    IteratorBuiltinsAssembler iterator_assembler(state());
-
-    // 1. Let iteratorRecord be ? GetIterator(items, method).
-    IteratorRecord iterator_record =
-        iterator_assembler.GetIterator(context, iterable, iterator_fn);
-
-    // 2. Let values be a new empty List.
-    GrowableFixedArray values(state());
-
-    Variable* vars[] = {values.var_array(), values.var_length(),
-                        values.var_capacity()};
-    Label loop_start(this, 3, vars), loop_end(this);
-    Goto(&loop_start);
-    // 3. Let next be true.
-    // 4. Repeat, while next is not false
-    BIND(&loop_start);
-    {
-      //  a. Set next to ? IteratorStep(iteratorRecord).
-      TNode<Object> next = CAST(
-          iterator_assembler.IteratorStep(context, iterator_record, &loop_end));
-      //  b. If next is not false, then
-      //   i. Let nextValue be ? IteratorValue(next).
-      TNode<Object> next_value =
-          CAST(iterator_assembler.IteratorValue(context, next));
-      //   ii. Append nextValue to the end of the List values.
-      values.Push(next_value);
-      Goto(&loop_start);
-    }
-    BIND(&loop_end);
-
-    // 5. Return values.
-    TNode<JSArray> js_array_values = values.ToJSArray(context);
-    created_list = js_array_values;
+    IterableToListSlowPath(context, iterable, iterator_fn, &created_list);
     Goto(&done);
   }
 

src/builtins/builtins-typed-array-gen.h

@@ -55,6 +55,10 @@ class TypedArrayBuiltinsAssembler : public BaseBuiltinsFromDSLAssembler {
                     TNode<Map> map, TNode<Smi> length,
                     TNode<Number> byte_offset);
 
+  void IterableToListSlowPath(TNode<Context> context, TNode<Object> iterable,
+                              TNode<Object> iterator_fn,
+                              Variable* created_list);
+
   TNode<Map> LoadMapForType(TNode<JSTypedArray> array);
   TNode<UintPtrT> CalculateExternalPointer(TNode<UintPtrT> backing_store,
                                            TNode<Number> byte_offset);

test/mjsunit/regress/regress-854066.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.
+
+oobArray = [];
+for (let i = 0; i < 1024 * 1024; ++i) {
+  oobArray[i] = 1.1;
+}
+floatArray = new Float64Array(oobArray.length);
+Float64Array.from.call(function(length) {
+  oobArray.length = 0;
+  return floatArray;
+}, oobArray);