[builtins] Fix Array.of crashes by setting length correctly

Before we can set the length of the created array in CSA, first check
that it's possible and will do what we want. I.e. check
a) that the length is writable
b) the backing store is not copy-on-write and
c) the old length is not greater than the new length (as otherwise later
insertion past the end could restore values from the original
constructor).

If not then fall back on Runtime::kSetProperty.

Bug: chromium:804177
Change-Id: Id0e452f9d160704bbd71e87a075ba4e3983729a7
Reviewed-on: https://chromium-review.googlesource.com/880922
Commit-Queue: Dan Elphick <delphick@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50818}

src/builtins/builtins-array-gen.cc

@@ -1841,7 +1841,7 @@ TF_BUILTIN(ArrayPrototypeFindIndex, ArrayBuiltinCodeStubAssembler) {
 }
 
 // ES #sec-array.of
-TF_BUILTIN(ArrayOf, ArrayBuiltinCodeStubAssembler) {
+TF_BUILTIN(ArrayOf, CodeStubAssembler) {
   TNode<Int32T> argc =
       UncheckedCast<Int32T>(Parameter(BuiltinDescriptor::kArgumentsCount));
   TNode<Smi> length = SmiFromWord32(argc);
@@ -1911,20 +1911,42 @@ TF_BUILTIN(ArrayOf, ArrayBuiltinCodeStubAssembler) {
                 1, ParameterMode::SMI_PARAMETERS, IndexAdvanceMode::kPost);
 
   {
-    Label is_js_array(this), is_not_js_array(this), next(this);
+    Label fast(this), runtime(this), next(this);
+    // Only set the length in this stub if
+    // 1) the array has fast elements,
+    // 2) the length is writable,
+    // 3) the new length is greater than or equal to the old length.
+
+    // 1) Check that the array has fast elements.
     // TODO(delphick): Consider changing this since it does an an unnecessary
     // check for SMIs.
-    // TODO(delphick): Also we could hoist this to after the array construction
-    // and copy the args into array in the same way as the Array constructor.
-    BranchIfFastJSArray(array, context, &is_js_array, &is_not_js_array);
+    // TODO(delphick): Also we could hoist this to after the array
+    // construction and copy the args into array in the same way as the Array
+    // constructor.
+    BranchIfFastJSArray(array, context, &fast, &runtime);
 
-    BIND(&is_js_array);
+    BIND(&fast);
     {
-      StoreObjectFieldNoWriteBarrier(array, JSArray::kLengthOffset, length);
+      TNode<JSArray> fast_array = CAST(array);
+
+      CSA_ASSERT(this, TaggedIsPositiveSmi(LoadJSArrayLength(fast_array)));
+      Node* old_length = LoadObjectField(fast_array, JSArray::kLengthOffset);
+
+      // 2) Ensure that the length is writable.
+      EnsureArrayLengthWritable(LoadMap(fast_array), &runtime);
+
+      // 3) If the created array already has a length greater than required,
+      //    then use the runtime to set the property as that will insert holes
+      //    into the excess elements and/or shrink the backing store.
+      GotoIf(SmiLessThan(length, old_length), &runtime);
+
+      StoreObjectFieldNoWriteBarrier(fast_array, JSArray::kLengthOffset,
+                                     length);
+
       Goto(&next);
     }
 
-    BIND(&is_not_js_array);
+    BIND(&runtime);
     {
       CallRuntime(Runtime::kSetProperty, context, static_cast<Node*>(array),
                   CodeStubAssembler::LengthStringConstant(), length,

test/mjsunit/regress/regress-804177.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.
+
+// Tests that insertion at the beginning via unshift won't crash when using a
+// constructor that creates an array larger than normal. (Also values inserted
+// by original constructor past the end should not survive into the result of
+// unshift).
+(function testUnshift() {
+  a = [1];
+  function f() {
+    return a;
+  }
+  b = Array.of.call(f);
+  b.unshift(2);
+  assertEquals(b, [2]);
+})();
+
+// Tests that insertion past the end won't expose values previously put into the
+// backing store by using a constructor that creates an array larger than normal.
+(function testInsertionPastEnd() {
+  a = [9,9,9,9];
+  function f() {
+    return a;
+  }
+  b = Array.of.call(f,1,2);
+  b[4] = 1;
+  assertEquals(b, [1, 2, undefined, undefined, 1]);
+})();
+
+// Tests that using Array.of with a constructor returning an object with an
+// unwriteable length throws a TypeError.
+(function testFrozenArrayThrows() {
+  function f() {
+    return Object.freeze([1,2,3]);
+  }
+  assertThrows(function() { Array.of.call(f); }, TypeError);
+})();