Fix: KeyedStoreGeneric must check for writable array length

This has been broken since forever.

BUG=v8:5669

Review-Url: https://codereview.chromium.org/2610343002
Cr-Commit-Position: refs/heads/master@{#42219}

src/builtins/builtins-array.cc

@@ -239,10 +239,9 @@ void Builtins::Generate_FastArrayPush(compiler::CodeAssemblerState* state) {
     Node* descriptors = assembler.LoadMapDescriptors(map);
     Node* details = assembler.LoadFixedArrayElement(
         descriptors, DescriptorArray::ToDetailsIndex(0));
-    mask = READ_ONLY << PropertyDetails::AttributesField::kShift;
-    Node* mask_node = assembler.SmiConstant(mask);
-    test = assembler.SmiAnd(details, mask_node);
-    assembler.GotoIf(assembler.WordEqual(test, mask_node), &runtime);
+    assembler.GotoIf(
+        assembler.IsSetSmi(details, PropertyDetails::kAttributesReadOnlyMask),
+        &runtime);
 
     arg_index.Bind(assembler.IntPtrConstant(0));
     kind = assembler.DecodeWord32<Map::ElementsKindBits>(bit_field2);

src/code-stub-assembler.h

@@ -798,6 +798,15 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
     return WordNotEqual(WordAnd(word, IntPtrConstant(mask)), IntPtrConstant(0));
   }
 
+  // Returns true if any of the mask's bit are set in the given Smi.
+  // Smi-encoding of the mask is performed implicitly!
+  Node* IsSetSmi(Node* smi, int untagged_mask) {
+    intptr_t mask_word = bit_cast<intptr_t>(Smi::FromInt(untagged_mask));
+    return WordNotEqual(
+        WordAnd(BitcastTaggedToWord(smi), IntPtrConstant(mask_word)),
+        IntPtrConstant(0));
+  }
+
   // Returns true if all of the |T|'s bits in given |word32| are clear.
   template <typename T>
   Node* IsClearWord32(Node* word32) {

src/ic/keyed-store-generic.cc

@@ -225,6 +225,15 @@ void KeyedStoreGenericAssembler::StoreElementWithCapacity(
   if (update_length != kDontChangeLength) {
     CSA_ASSERT(this, Word32Equal(LoadMapInstanceType(receiver_map),
                                  Int32Constant(JS_ARRAY_TYPE)));
+    // Check if the length property is writable. The fast check is only
+    // supported for fast properties.
+    GotoIf(IsDictionaryMap(receiver_map), slow);
+    // The length property is non-configurable, so it's guaranteed to always
+    // be the first property.
+    Node* descriptors = LoadMapDescriptors(receiver_map);
+    Node* details =
+        LoadFixedArrayElement(descriptors, DescriptorArray::ToDetailsIndex(0));
+    GotoIf(IsSetSmi(details, PropertyDetails::kAttributesReadOnlyMask), slow);
   }
   STATIC_ASSERT(FixedArray::kHeaderSize == FixedDoubleArray::kHeaderSize);
   const int kHeaderSize = FixedArray::kHeaderSize - kHeapObjectTag;

test/mjsunit/regress/regress-5669.js

+// Copyright 2017 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 f(a, i, v) { a[i] = v; }
+f("make it generic", 0, 0);
+
+var a = new Array();
+Object.defineProperty(a, "length", {value: 3, writable: false});
+print(JSON.stringify(Object.getOwnPropertyDescriptor(a, "length")));
+assertEquals(3, a.length);
+f(a, 3, 3);
+assertFalse(Object.getOwnPropertyDescriptor(a, "length").writable);
+assertEquals(3, a.length);
+
+var b = new Array();
+b.length = 3;
+Object.freeze(b);
+assertEquals(3, b.length);
+f(b, 3, 3);
+assertEquals(3, b.length);