From b5da57a06de8791693c248b7aafc734861a3785d Mon Sep 17 00:00:00 2001
From: Dan Elphick <delphick@chromium.org>
Date: Wed, 14 Mar 2018 10:02:08 +0000
Subject: [PATCH] [builtins] Fix OOB read/write using Array.from

Always use the runtime to set the length on an array if it doesn't match
the expected length after populating it using Array.from.

Bug: chromium:821137
Change-Id: I5a730db58de61ba789040e6dfc815d6067fbae64
Reviewed-on: https://chromium-review.googlesource.com/962222
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Dan Elphick <delphick@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51919}
---
 src/builtins/builtins-array-gen.cc     | 11 +++++++----
 test/mjsunit/regress/regress-821137.js | 27 ++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 test/mjsunit/regress/regress-821137.js

diff --git a/src/builtins/builtins-array-gen.cc b/src/builtins/builtins-array-gen.cc
index dcf3be4750..3a7434237e 100644
--- a/src/builtins/builtins-array-gen.cc
+++ b/src/builtins/builtins-array-gen.cc
@@ -1945,10 +1945,13 @@ class ArrayPopulatorAssembler : public CodeStubAssembler {
   void GenerateSetLength(TNode<Context> context, TNode<Object> array,
                          TNode<Number> length) {
     Label fast(this), runtime(this), done(this);
+    // TODO(delphick): We should be able to skip the fast set altogether, if the
+    // length already equals the expected length, which it always is now on the
+    // fast path.
     // 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.
+    // 3) the new length is equal to the old length.
 
     // 1) Check that the array has fast elements.
     // TODO(delphick): Consider changing this since it does an an unnecessary
@@ -1970,10 +1973,10 @@ class ArrayPopulatorAssembler : public CodeStubAssembler {
       // BranchIfFastJSArray above.
       EnsureArrayLengthWritable(LoadMap(fast_array), &runtime);
 
-      // 3) If the created array already has a length greater than required,
+      // 3) If the created array's length does not match the required length,
       //    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_smi, old_length), &runtime);
+      //    into excess elements or shrink the backing store as appropriate.
+      GotoIf(SmiNotEqual(length_smi, old_length), &runtime);
 
       StoreObjectFieldNoWriteBarrier(fast_array, JSArray::kLengthOffset,
                                      length_smi);
diff --git a/test/mjsunit/regress/regress-821137.js b/test/mjsunit/regress/regress-821137.js
new file mode 100644
index 0000000000..639b3b998a
--- /dev/null
+++ b/test/mjsunit/regress/regress-821137.js
@@ -0,0 +1,27 @@
+// 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 creating an iterator that shrinks the array populated by
+// Array.from does not lead to out of bounds writes.
+let oobArray = [];
+let maxSize = 1028 * 8;
+Array.from.call(function() { return oobArray }, {[Symbol.iterator] : _ => (
+  {
+    counter : 0,
+    next() {
+      let result = this.counter++;
+      if (this.counter > maxSize) {
+        oobArray.length = 0;
+        return {done: true};
+      } else {
+        return {value: result, done: false};
+      }
+    }
+  }
+) });
+assertEquals(oobArray.length, maxSize);
+
+// iterator reset the length to 0 just before returning done, so this will crash
+// if the backing store was not resized correctly.
+oobArray[oobArray.length - 1] = 0x41414141;