[runtime] Harden JSFunction::CalculateInstanceSizeHelper(...)

Bug: chromium:808192
Change-Id: I80136d291d5c21c311903bffc96d86d109f5cdc9
Reviewed-on: https://chromium-review.googlesource.com/902103
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51255}

src/objects.cc

@@ -12985,6 +12985,56 @@ void JSFunction::EnsureHasInitialMap(Handle<JSFunction> function) {
   map->StartInobjectSlackTracking();
 }
 
+namespace {
+bool FastInitializeDerivedMap(Isolate* isolate, Handle<JSFunction> new_target,
+                              Handle<JSFunction> constructor,
+                              Handle<Map> constructor_initial_map) {
+  // Check that |function|'s initial map still in sync with the |constructor|,
+  // otherwise we must create a new initial map for |function|.
+  if (new_target->has_initial_map() &&
+      new_target->initial_map()->GetConstructor() == *constructor) {
+    DCHECK(new_target->instance_prototype()->IsJSReceiver());
+    return true;
+  }
+  InstanceType instance_type = constructor_initial_map->instance_type();
+  DCHECK(CanSubclassHaveInobjectProperties(instance_type));
+  // Create a new map with the size and number of in-object properties
+  // suggested by |function|.
+
+  // Link initial map and constructor function if the new.target is actually a
+  // subclass constructor.
+  if (!IsDerivedConstructor(new_target->shared()->kind())) return false;
+
+  int instance_size;
+  int in_object_properties;
+  int embedder_fields =
+      JSObject::GetEmbedderFieldCount(*constructor_initial_map);
+  bool success = JSFunction::CalculateInstanceSizeForDerivedClass(
+      new_target, instance_type, embedder_fields, &instance_size,
+      &in_object_properties);
+
+  Handle<Map> map;
+  if (success) {
+    int pre_allocated = constructor_initial_map->GetInObjectProperties() -
+                        constructor_initial_map->UnusedPropertyFields();
+    CHECK_LE(constructor_initial_map->instance_size(), instance_size);
+    int unused_property_fields = in_object_properties - pre_allocated;
+    map = Map::CopyInitialMap(constructor_initial_map, instance_size,
+                              in_object_properties, unused_property_fields);
+  } else {
+    map = Map::CopyInitialMap(constructor_initial_map);
+  }
+  map->set_new_target_is_base(false);
+  Handle<Object> prototype(new_target->instance_prototype(), isolate);
+  JSFunction::SetInitialMap(new_target, map, prototype);
+  DCHECK(new_target->instance_prototype()->IsJSReceiver());
+  map->SetConstructor(*constructor);
+  map->set_construction_counter(Map::kNoSlackTracking);
+  map->StartInobjectSlackTracking();
+  return true;
+}
+
+}  // namespace
 
 // static
 MaybeHandle<Map> JSFunction::GetDerivedMap(Isolate* isolate,
@@ -12995,55 +13045,16 @@ MaybeHandle<Map> JSFunction::GetDerivedMap(Isolate* isolate,
   Handle<Map> constructor_initial_map(constructor->initial_map(), isolate);
   if (*new_target == *constructor) return constructor_initial_map;
 
+  Handle<Map> result_map;
   // Fast case, new.target is a subclass of constructor. The map is cacheable
   // (and may already have been cached). new.target.prototype is guaranteed to
   // be a JSReceiver.
   if (new_target->IsJSFunction()) {
     Handle<JSFunction> function = Handle<JSFunction>::cast(new_target);
-
-    // Check that |function|'s initial map still in sync with the |constructor|,
-    // otherwise we must create a new initial map for |function|.
-    if (function->has_initial_map() &&
-        function->initial_map()->GetConstructor() == *constructor) {
+    if (FastInitializeDerivedMap(isolate, function, constructor,
+                                 constructor_initial_map)) {
       return handle(function->initial_map(), isolate);
     }
-
-    // Create a new map with the size and number of in-object properties
-    // suggested by |function|.
-
-    // Link initial map and constructor function if the new.target is actually a
-    // subclass constructor.
-    if (IsDerivedConstructor(function->shared()->kind())) {
-      Handle<Object> prototype(function->instance_prototype(), isolate);
-      InstanceType instance_type = constructor_initial_map->instance_type();
-      DCHECK(CanSubclassHaveInobjectProperties(instance_type));
-      int embedder_fields =
-          JSObject::GetEmbedderFieldCount(*constructor_initial_map);
-      int pre_allocated = constructor_initial_map->GetInObjectProperties() -
-                          constructor_initial_map->UnusedPropertyFields();
-      int instance_size;
-      int in_object_properties;
-      bool success = CalculateInstanceSizeForDerivedClass(
-          function, instance_type, embedder_fields, &instance_size,
-          &in_object_properties);
-
-      int unused_property_fields = in_object_properties - pre_allocated;
-
-      Handle<Map> map;
-      if (success) {
-        map = Map::CopyInitialMap(constructor_initial_map, instance_size,
-                                  in_object_properties, unused_property_fields);
-      } else {
-        map = Map::CopyInitialMap(constructor_initial_map);
-      }
-      map->set_new_target_is_base(false);
-
-      JSFunction::SetInitialMap(function, map, prototype);
-      map->SetConstructor(*constructor);
-      map->set_construction_counter(Map::kNoSlackTracking);
-      map->StartInobjectSlackTracking();
-      return map;
-    }
   }
 
   // Slow path, new.target is either a proxy or can't cache the map.
@@ -13085,7 +13096,7 @@ MaybeHandle<Map> JSFunction::GetDerivedMap(Isolate* isolate,
 
   Handle<Map> map = Map::CopyInitialMap(constructor_initial_map);
   map->set_new_target_is_base(false);
-  DCHECK(prototype->IsJSReceiver());
+  CHECK(prototype->IsJSReceiver());
   if (map->prototype() != *prototype) Map::SetPrototype(map, prototype);
   map->SetConstructor(*constructor);
   return map;
@@ -13779,15 +13790,17 @@ void JSFunction::CalculateInstanceSizeHelper(InstanceType instance_type,
                                              int* instance_size,
                                              int* in_object_properties) {
   int header_size = JSObject::GetHeaderSize(instance_type, has_prototype_slot);
-  DCHECK_LE(requested_embedder_fields,
-            (JSObject::kMaxInstanceSize - header_size) >> kPointerSizeLog2);
+  int max_nof_fields =
+      (JSObject::kMaxInstanceSize - header_size) >> kPointerSizeLog2;
+  CHECK_LE(max_nof_fields, JSObject::kMaxInObjectProperties);
+  *in_object_properties = Min(requested_in_object_properties, max_nof_fields);
+  CHECK_LE(requested_embedder_fields, max_nof_fields - *in_object_properties);
   *instance_size =
-      Min(header_size +
-              ((requested_embedder_fields + requested_in_object_properties)
-               << kPointerSizeLog2),
-          JSObject::kMaxInstanceSize);
-  *in_object_properties = ((*instance_size - header_size) >> kPointerSizeLog2) -
-                          requested_embedder_fields;
+      header_size +
+      ((requested_embedder_fields + *in_object_properties) << kPointerSizeLog2);
+  CHECK_EQ(*in_object_properties,
+           ((*instance_size - header_size) >> kPointerSizeLog2) -
+               requested_embedder_fields);
 }
 
 // static
@@ -13797,7 +13810,6 @@ bool JSFunction::CalculateInstanceSizeForDerivedClass(
     int* in_object_properties) {
   Isolate* isolate = function->GetIsolate();
   int expected_nof_properties = 0;
-  bool result = true;
   for (PrototypeIterator iter(isolate, function, kStartAtReceiver);
        !iter.IsAtEnd(); iter.Advance()) {
     Handle<JSReceiver> current =
@@ -13810,21 +13822,24 @@ bool JSFunction::CalculateInstanceSizeForDerivedClass(
     if (shared->is_compiled() ||
         Compiler::Compile(func, Compiler::CLEAR_EXCEPTION)) {
       DCHECK(shared->is_compiled());
-      expected_nof_properties += shared->expected_nof_properties();
+      int count = shared->expected_nof_properties();
+      // Check that the estimate is sane.
+      if (expected_nof_properties <= JSObject::kMaxInObjectProperties - count) {
+        expected_nof_properties += count;
+      } else {
+        expected_nof_properties = JSObject::kMaxInObjectProperties;
+      }
     } else if (!shared->is_compiled()) {
       // In case there was a compilation error for the constructor we will
       // throw an error during instantiation. Hence we directly return 0;
-      result = false;
-      break;
-    }
-    if (!IsDerivedConstructor(shared->kind())) {
-      break;
+      return false;
     }
+    if (!IsDerivedConstructor(shared->kind())) break;
   }
   CalculateInstanceSizeHelper(instance_type, true, requested_embedder_fields,
                               expected_nof_properties, instance_size,
                               in_object_properties);
-  return result;
+  return true;
 }
 
 

src/objects.h

@@ -2707,6 +2707,7 @@ class JSObject: public JSReceiver {
   STATIC_ASSERT(kHeaderSize == Internals::kJSObjectHeaderSize);
   static const int kMaxInObjectProperties =
       (kMaxInstanceSize - kHeaderSize) >> kPointerSizeLog2;
+  STATIC_ASSERT(kMaxInObjectProperties <= kMaxNumberOfDescriptors);
 
   class BodyDescriptor;
   // No weak fields.

test/cctest/test-inobject-slack-tracking.cc

@@ -622,6 +622,12 @@ static void TestSubclassChain(const std::vector<int>& hierarchy_desc) {
   TestClassHierarchy(hierarchy_desc, static_cast<int>(hierarchy_desc.size()));
 }
 
+TEST(Subclasses) {
+  std::vector<int> hierarchy_desc;
+  hierarchy_desc.push_back(50);
+  hierarchy_desc.push_back(128);
+  TestSubclassChain(hierarchy_desc);
+}
 
 TEST(LongSubclassChain1) {
   std::vector<int> hierarchy_desc;

test/mjsunit/es6/destructuring-assignment.js

@@ -513,25 +513,31 @@ assertEquals(oz, [1, 2, 3, 4, 5]);
   }
 
   function FakeNewTarget() {}
-  assertEquals(undefined, ReturnNewTarget1());
-  assertEquals(ReturnNewTarget1, new ReturnNewTarget1());
-  assertEquals(FakeNewTarget,
-               Reflect.construct(ReturnNewTarget1, [], FakeNewTarget));
-
-  assertEquals(undefined, ReturnNewTarget2());
-  assertEquals(ReturnNewTarget2, new ReturnNewTarget2());
-  assertEquals(FakeNewTarget,
-               Reflect.construct(ReturnNewTarget2, [], FakeNewTarget));
-
-  assertEquals(undefined, ReturnNewTarget3());
-  assertEquals(ReturnNewTarget3, new ReturnNewTarget3());
-  assertEquals(FakeNewTarget,
-               Reflect.construct(ReturnNewTarget3, [], FakeNewTarget));
-
-  assertEquals(undefined, ReturnNewTarget4());
-  assertEquals(ReturnNewTarget4, new ReturnNewTarget4());
-  assertEquals(FakeNewTarget,
-               Reflect.construct(ReturnNewTarget4, [], FakeNewTarget));
+
+  function construct() {
+    assertEquals(undefined, ReturnNewTarget1());
+    assertEquals(ReturnNewTarget1, new ReturnNewTarget1());
+    assertEquals(FakeNewTarget,
+                 Reflect.construct(ReturnNewTarget1, [], FakeNewTarget));
+
+    assertEquals(undefined, ReturnNewTarget2());
+    assertEquals(ReturnNewTarget2, new ReturnNewTarget2());
+    assertEquals(FakeNewTarget,
+                 Reflect.construct(ReturnNewTarget2, [], FakeNewTarget));
+
+    assertEquals(undefined, ReturnNewTarget3());
+    assertEquals(ReturnNewTarget3, new ReturnNewTarget3());
+    assertEquals(FakeNewTarget,
+                 Reflect.construct(ReturnNewTarget3, [], FakeNewTarget));
+
+    assertEquals(undefined, ReturnNewTarget4());
+    assertEquals(ReturnNewTarget4, new ReturnNewTarget4());
+    assertEquals(FakeNewTarget,
+                 Reflect.construct(ReturnNewTarget4, [], FakeNewTarget));
+  }
+  construct();
+  FakeNewTarget.prototype = 1;
+  construct();
 })();
 
 (function testSuperCall() {

test/mjsunit/mjsunit.status

@@ -86,6 +86,9 @@
   # TODO(arm): This seems to flush out a bug on arm with simulator.
   'array-constructor': [PASS, SLOW, ['arch == arm and simulator == True', SKIP]],
 
+  # Very slow test
+  'regress/regress-crbug-808192' : [PASS, NO_VARIANTS, ['arch == arm or arch == arm64 or arch == android_arm or arch == android_arm64 or arch == mipsel or arch == mips64el or arch == mips64 or arch == mips', SKIP]],
+
   # Very slow on ARM and MIPS, contains no architecture dependent code.
   'unicode-case-overoptimization': [PASS, NO_VARIANTS, ['arch == arm or arch == arm64 or arch == android_arm or arch == android_arm64 or arch == mipsel or arch == mips64el or arch == mips64 or arch == mips', SKIP]],
   'regress/regress-3976': [PASS, NO_VARIANTS, ['arch == arm or arch == arm64 or arch == android_arm or arch == android_arm64 or arch == mipsel or arch == mips64el or arch == mips64 or arch == mips', SKIP]],

test/mjsunit/regress/regress-crbug-808192.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.
+
+// TODO(cbruni): enable always opt once v8:7438
+// Flags: --expose-gc --no-always-opt
+
+const f = eval(`(function f(i) {
+  if (i == 0) {
+    class Derived extends Object {
+      constructor() {
+        super();
+        ${"this.a=1;".repeat(0x3fffe-8)}
+      }
+    }
+    return Derived;
+  }
+
+  class DerivedN extends f(i-1) {
+    constructor() {
+      super();
+      ${"this.a=1;".repeat(0x40000-8)}
+    }
+  }
+
+  return DerivedN;
+})`);
+
+let a = new (f(0x7ff))();
+a.a = 1;
+gc();
+assertEquals(1, a.a);