Reland "[VM - Runtime] Return nullptr when allocating a FinalizablePe…

…rsistentHandle fails"

This is a reland of commit b8d4e24

How the failures were fixed:
1. My ExternalSizeLimit test crashed on msvc because I was using a
0-sized array. I have now changed that array to have size 1.
2. My ExternalSizeLimit test crashed on x64c because
ExternalTypedData::MaxElements(kExternalTypedDataUint8ArrayCid) is much
smaller than kMaxAddrSpaceMB/4 on x64c. I now call
ExternalTypedData::New() with a length argument of 1, and just pretend
that the external allocations are larger when calling
FinalizablePersistentHandle::New().

Original change's description:
> [VM - Runtime] Return nullptr when allocating a
> FinalizablePersistentHandle fails
>
> This CL adds checks to ensure that the tracked total size of
> externally allocated objects never exceeds the amount of memory on the
> system. When the limit is exceeded, then
> FinalizablePersistentHandle::New() will return nullptr.
>
> Resolves #49332
>
> TEST=ci
>
> Change-Id: Ib6cc92325b1d5efcb2965098fa45cfecc90995e3
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256201
> Reviewed-by: Ben Konyi <[email protected]>
> Commit-Queue: Derek Xu <[email protected]>
> Reviewed-by: Siva Annamalai <[email protected]>

TEST=I ran the tryjobs for the configurations that broke CI.

Change-Id: I813aa74667c59a4dbec7f53440ca8d0bf21256ce
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/256973
Reviewed-by: Ben Konyi <[email protected]>
Reviewed-by: Siva Annamalai <[email protected]>
Commit-Queue: Derek Xu <[email protected]>

runtime/lib/ffi.cc

@@ -274,8 +274,10 @@ DEFINE_FFI_NATIVE_ENTRY(FinalizerEntry_SetExternalSize,
   }
   // The next call cannot be in safepoint.
   if (external_size_diff > 0) {
-    thread->isolate_group()->heap()->AllocatedExternal(external_size_diff,
-                                                       space);
+    if (!thread->isolate_group()->heap()->AllocatedExternal(external_size_diff,
+                                                            space)) {
+      Exceptions::ThrowOOM();
+    }
   } else {
     thread->isolate_group()->heap()->FreedExternal(-external_size_diff, space);
   }

runtime/lib/isolate.cc

@@ -244,8 +244,7 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
       case kFloat64x2Cid:
         continue;
 
-      case kArrayCid:
-      {
+      case kArrayCid: {
         array ^= Array::RawCast(raw);
         visitor.VisitObject(array.GetTypeArguments());
         const intptr_t batch_size = (2 << 14) - 1;
@@ -1303,10 +1302,12 @@ DEFINE_NATIVE_ENTRY(TransferableTypedData_materialize, 0, 1) {
   const ExternalTypedData& typed_data = ExternalTypedData::Handle(
       ExternalTypedData::New(kExternalTypedDataUint8ArrayCid, data, length,
                              thread->heap()->SpaceForExternal(length)));
-  FinalizablePersistentHandle::New(thread->isolate_group(), typed_data,
-                                   /* peer= */ data,
-                                   &ExternalTypedDataFinalizer, length,
-                                   /*auto_delete=*/true);
+  FinalizablePersistentHandle* finalizable_ref =
+      FinalizablePersistentHandle::New(thread->isolate_group(), typed_data,
+                                       /* peer= */ data,
+                                       &ExternalTypedDataFinalizer, length,
+                                       /*auto_delete=*/true);
+  ASSERT(finalizable_ref != nullptr);
   return typed_data.ptr();
 }
 

runtime/vm/dart_api_impl.cc

@@ -1036,7 +1036,9 @@ static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
                                        callback, external_allocation_size,
                                        /*auto_delete=*/false);
-  return finalizable_ref->ApiWeakPersistentHandle();
+  return finalizable_ref == nullptr
+             ? nullptr
+             : finalizable_ref->ApiWeakPersistentHandle();
 }
 
 static Dart_WeakPersistentHandle AllocateWeakPersistentHandle(
@@ -1070,7 +1072,8 @@ static Dart_FinalizableHandle AllocateFinalizableHandle(
       FinalizablePersistentHandle::New(thread->isolate_group(), ref, peer,
                                        callback, external_allocation_size,
                                        /*auto_delete=*/true);
-  return finalizable_ref->ApiFinalizableHandle();
+  return finalizable_ref == nullptr ? nullptr
+                                    : finalizable_ref->ApiFinalizableHandle();
 }
 
 static Dart_FinalizableHandle AllocateFinalizableHandle(

runtime/vm/dart_api_state.h

@@ -6,13 +6,13 @@
 #define RUNTIME_VM_DART_API_STATE_H_
 
 #include "include/dart_api.h"
-
 #include "platform/utils.h"
 #include "vm/bitfield.h"
 #include "vm/dart_api_impl.h"
 #include "vm/flags.h"
 #include "vm/growable_array.h"
 #include "vm/handles.h"
+#include "vm/handles_impl.h"
 #include "vm/heap/weak_table.h"
 #include "vm/object.h"
 #include "vm/os.h"
@@ -21,8 +21,6 @@
 #include "vm/thread_pool.h"
 #include "vm/visitor.h"
 
-#include "vm/handles_impl.h"
-
 namespace dart {
 
 // Implementation of Zone support for very fast allocation of small chunks
@@ -223,14 +221,19 @@ class FinalizablePersistentHandle {
     return ExternalSizeInWordsBits::decode(external_data_) * kWordSize;
   }
 
-  void SetExternalSize(intptr_t size, IsolateGroup* isolate_group) {
-    ASSERT(size >= 0);
+  bool SetExternalSize(intptr_t size, IsolateGroup* isolate_group) {
+    // This method only behaves correctly when external_size() has not been
+    // previously modified.
+    ASSERT(external_size() == 0);
+    if (size < 0 || (size >> kWordSizeLog2) > kMaxAddrSpaceInWords) {
+      return false;
+    }
     set_external_size(size);
     if (SpaceForExternal() == Heap::kNew) {
       SetExternalNewSpaceBit();
     }
-    isolate_group->heap()->AllocatedExternal(external_size(),
-                                             SpaceForExternal());
+    return isolate_group->heap()->AllocatedExternal(external_size(),
+                                                    SpaceForExternal());
   }
   void UpdateExternalSize(intptr_t size, IsolateGroup* isolate_group) {
     ASSERT(size >= 0);
@@ -803,14 +806,17 @@ inline FinalizablePersistentHandle* FinalizablePersistentHandle::New(
     intptr_t external_size,
     bool auto_delete) {
   ApiState* state = isolate_group->api_state();
-  ASSERT(state != NULL);
+  ASSERT(state != nullptr);
   FinalizablePersistentHandle* ref = state->AllocateWeakPersistentHandle();
   ref->set_ptr(object);
   ref->set_peer(peer);
   ref->set_callback(callback);
   ref->set_auto_delete(auto_delete);
   // This may trigger GC, so it must be called last.
-  ref->SetExternalSize(external_size, isolate_group);
+  if (!(ref->SetExternalSize(external_size, isolate_group))) {
+    state->FreeWeakPersistentHandle(ref);
+    return nullptr;
+  }
   return ref;
 }
 

runtime/vm/globals.h

@@ -44,7 +44,11 @@ static constexpr int kCompressedWordSizeLog2 = kWordSizeLog2;
 typedef uintptr_t compressed_uword;
 typedef intptr_t compressed_word;
 #endif
-const int kMaxAddrSpaceMB = (kWordSize <= 4) ? 4096 : kMaxInt;
+// 32-bit: 2^32 addresses => kMaxAddrSpaceMB = 2^(32 - MBLog2) = 2^12 MB
+// 64-bit: 2^48 addresses => kMaxAddrSpaceMB = 2^(48 - MBLog2) = 2^28 MB
+const intptr_t kMaxAddrSpaceMB = (kWordSize <= 4) ? 4096 : 268435456;
+const intptr_t kMaxAddrSpaceInWords = kMaxAddrSpaceMB >> kWordSizeLog2
+                                                             << MBLog2;
 
 // Number of bytes per BigInt digit.
 const intptr_t kBytesPerBigIntDigit = 4;

runtime/vm/heap/heap.cc

@@ -107,7 +107,7 @@ uword Heap::AllocateOld(Thread* thread, intptr_t size, OldPage::PageType type) {
     }
     // All GC tasks finished without allocating successfully. Collect both
     // generations.
-    CollectMostGarbage(GCReason::kOldSpace, /*compact=*/ false);
+    CollectMostGarbage(GCReason::kOldSpace, /*compact=*/false);
     addr = old_space_.TryAllocate(size, type);
     if (addr != 0) {
       return addr;
@@ -124,7 +124,7 @@ uword Heap::AllocateOld(Thread* thread, intptr_t size, OldPage::PageType type) {
       return addr;
     }
     // Before throwing an out-of-memory error try a synchronous GC.
-    CollectAllGarbage(GCReason::kOldSpace, /*compact=*/ true);
+    CollectAllGarbage(GCReason::kOldSpace, /*compact=*/true);
     WaitForSweeperTasks(thread);
   }
   uword addr = old_space_.TryAllocate(size, type, PageSpace::kForceGrowth);
@@ -146,12 +146,16 @@ uword Heap::AllocateOld(Thread* thread, intptr_t size, OldPage::PageType type) {
   return 0;
 }
 
-void Heap::AllocatedExternal(intptr_t size, Space space) {
+bool Heap::AllocatedExternal(intptr_t size, Space space) {
   if (space == kNew) {
-    new_space_.AllocatedExternal(size);
+    if (!new_space_.AllocatedExternal(size)) {
+      return false;
+    }
   } else {
     ASSERT(space == kOld);
-    old_space_.AllocatedExternal(size);
+    if (!old_space_.AllocatedExternal(size)) {
+      return false;
+    }
   }
 
   Thread* thread = Thread::Current();
@@ -160,6 +164,7 @@ void Heap::AllocatedExternal(intptr_t size, Space space) {
   } else {
     // Check delayed until Dart_TypedDataRelease/~ForceGrowthScope.
   }
+  return true;
 }
 
 void Heap::FreedExternal(intptr_t size, Space space) {

runtime/vm/heap/heap.h

@@ -78,8 +78,10 @@ class Heap {
     return 0;
   }
 
-  // Track external data.
-  void AllocatedExternal(intptr_t size, Space space);
+  // Tracks an external allocation. Returns false without tracking the
+  // allocation if it will make the total external size exceed
+  // kMaxAddrSpaceInWords.
+  bool AllocatedExternal(intptr_t size, Space space);
   void FreedExternal(intptr_t size, Space space);
   // Move external size from new to old space. Does not by itself trigger GC.
   void PromotedExternal(intptr_t size);

runtime/vm/heap/heap_test.cc

@@ -674,6 +674,76 @@ ISOLATE_UNIT_TEST_CASE(ExternalAllocationStats) {
               heap->new_space()->ExternalInWords() * kWordSize);
   }
 }
+
+ISOLATE_UNIT_TEST_CASE(ExternalSizeLimit) {
+  // This test checks that the tracked total size of external data never exceeds
+  // the amount of memory on the system. To accomplish this, the test performs
+  // five calls to FinalizablePersistentHandle::New(), all supplying a size
+  // argument that is barely (16 bytes) less than a quarter of kMaxAddrSpaceMB.
+  // So, we expect the first four calls to succeed, and the fifth one to return
+  // nullptr.
+
+  auto isolate_group = thread->isolate_group();
+  Heap* heap = isolate_group->heap();
+
+  // We declare an array of only length 1 here to get around the limit of
+  // ExternalTypedData::MaxElements(kExternalTypedDataUint8ArrayCid). Below, we
+  // pretend that the length is longer when calling
+  // FinalizablePersistentHandle::New(), which is what updates the external size
+  // tracker.
+  const intptr_t data_length = 1;
+  uint8_t data[data_length] = {0};
+  const ExternalTypedData& external_typed_data_1 =
+      ExternalTypedData::Handle(ExternalTypedData::New(
+          kExternalTypedDataUint8ArrayCid, data, data_length, Heap::kOld));
+  const ExternalTypedData& external_typed_data_2 =
+      ExternalTypedData::Handle(ExternalTypedData::New(
+          kExternalTypedDataUint8ArrayCid, data, data_length, Heap::kOld));
+  const ExternalTypedData& external_typed_data_3 =
+      ExternalTypedData::Handle(ExternalTypedData::New(
+          kExternalTypedDataUint8ArrayCid, data, data_length, Heap::kOld));
+  const ExternalTypedData& external_typed_data_4 =
+      ExternalTypedData::Handle(ExternalTypedData::New(
+          kExternalTypedDataUint8ArrayCid, data, data_length, Heap::kOld));
+  const ExternalTypedData& external_typed_data_5 =
+      ExternalTypedData::Handle(ExternalTypedData::New(
+          kExternalTypedDataUint8ArrayCid, data, data_length, Heap::kOld));
+
+  // A size that is less than a quarter of kMaxAddrSpaceMB is used because it
+  // needs to be less than or equal to std::numeric_limits<intptr_t>::max().
+  const intptr_t external_allocation_size =
+      (intptr_t{kMaxAddrSpaceMB / 4} << MBLog2) - 16;
+  EXPECT_NOTNULL(FinalizablePersistentHandle::New(
+      isolate_group, external_typed_data_1, nullptr, NoopFinalizer,
+      external_allocation_size,
+      /*auto_delete=*/true));
+  EXPECT_LT(heap->old_space()->ExternalInWords(), kMaxAddrSpaceInWords);
+
+  EXPECT_NOTNULL(FinalizablePersistentHandle::New(
+      isolate_group, external_typed_data_2, nullptr, NoopFinalizer,
+      external_allocation_size,
+      /*auto_delete=*/true));
+  EXPECT_LT(heap->old_space()->ExternalInWords(), kMaxAddrSpaceInWords);
+
+  EXPECT_NOTNULL(FinalizablePersistentHandle::New(
+      isolate_group, external_typed_data_3, nullptr, NoopFinalizer,
+      external_allocation_size,
+      /*auto_delete=*/true));
+  EXPECT_LT(heap->old_space()->ExternalInWords(), kMaxAddrSpaceInWords);
+
+  EXPECT_NOTNULL(FinalizablePersistentHandle::New(
+      isolate_group, external_typed_data_4, nullptr, NoopFinalizer,
+      external_allocation_size,
+      /*auto_delete=*/true));
+  EXPECT_LT(heap->old_space()->ExternalInWords(), kMaxAddrSpaceInWords);
+
+  EXPECT_NULLPTR(FinalizablePersistentHandle::New(
+      isolate_group, external_typed_data_5, nullptr, NoopFinalizer,
+      external_allocation_size,
+      /*auto_delete=*/true));
+  // Check that the external size is indeed protected from overflowing.
+  EXPECT_LT(heap->old_space()->ExternalInWords(), kMaxAddrSpaceInWords);
+}
 #endif  // !defined(PRODUCT)
 
 ISOLATE_UNIT_TEST_CASE(ArrayTruncationRaces) {

runtime/vm/heap/pages.h

@@ -438,10 +438,19 @@ class PageSpace {
     allocated_black_in_words_.fetch_add(size >> kWordSizeLog2);
   }
 
-  void AllocatedExternal(intptr_t size) {
+  // Tracks an external allocation by incrementing the old space's total
+  // external size tracker. Returns false without incrementing the tracker if
+  // this allocation will make it exceed kMaxAddrSpaceInWords.
+  bool AllocatedExternal(intptr_t size) {
     ASSERT(size >= 0);
     intptr_t size_in_words = size >> kWordSizeLog2;
-    usage_.external_in_words += size_in_words;
+    intptr_t next_external_in_words = usage_.external_in_words + size_in_words;
+    if (next_external_in_words < 0 ||
+        next_external_in_words > kMaxAddrSpaceInWords) {
+      return false;
+    }
+    usage_.external_in_words = next_external_in_words;
+    return true;
   }
   void FreedExternal(intptr_t size) {
     ASSERT(size >= 0);

runtime/vm/heap/scavenger.h

@@ -335,10 +335,20 @@ class Scavenger {
   void PrintToJSONObject(JSONObject* object) const;
 #endif  // !PRODUCT
 
-  void AllocatedExternal(intptr_t size) {
+  // Tracks an external allocation by incrementing the new space's total
+  // external size tracker. Returns false without incrementing the tracker if
+  // this allocation will make it exceed kMaxAddrSpaceInWords.
+  bool AllocatedExternal(intptr_t size) {
     ASSERT(size >= 0);
+    intptr_t next_external_size_in_words =
+        (external_size_ >> kWordSizeLog2) + (size >> kWordSizeLog2);
+    if (next_external_size_in_words < 0 ||
+        next_external_size_in_words > kMaxAddrSpaceInWords) {
+      return false;
+    }
     external_size_ += size;
     ASSERT(external_size_ >= 0);
+    return true;
   }
   void FreedExternal(intptr_t size) {
     ASSERT(size >= 0);

runtime/vm/object.cc

@@ -23788,10 +23788,13 @@ static FinalizablePersistentHandle* AddFinalizer(const Object& referent,
                                                  void* peer,
                                                  Dart_HandleFinalizer callback,
                                                  intptr_t external_size) {
-  ASSERT(callback != NULL);
-  return FinalizablePersistentHandle::New(IsolateGroup::Current(), referent,
-                                          peer, callback, external_size,
-                                          /*auto_delete=*/true);
+  ASSERT(callback != nullptr);
+  FinalizablePersistentHandle* finalizable_ref =
+      FinalizablePersistentHandle::New(IsolateGroup::Current(), referent, peer,
+                                       callback, external_size,
+                                       /*auto_delete=*/true);
+  ASSERT(finalizable_ref != nullptr);
+  return finalizable_ref;
 }
 
 StringPtr String::Transform(int32_t (*mapping)(int32_t ch),
@@ -25642,10 +25645,12 @@ TransferableTypedDataPtr TransferableTypedData::New(uint8_t* data,
   }
   // Set up finalizer so it frees allocated memory if handle is
   // garbage-collected.
-  peer->set_handle(
+  FinalizablePersistentHandle* finalizable_ref =
       FinalizablePersistentHandle::New(thread->isolate_group(), result, peer,
                                        &TransferableTypedDataFinalizer, length,
-                                       /*auto_delete=*/true));
+                                       /*auto_delete=*/true);
+  ASSERT(finalizable_ref != nullptr);
+  peer->set_handle(finalizable_ref);
 
   return result.ptr();
 }

runtime/vm/object_graph_copy.cc

@@ -595,9 +595,12 @@ class ForwardMapBase {
 
     // Move the handle itself to the new object.
     fpeer->handle()->EnsureFreedExternal(thread_->isolate_group());
-    tpeer->set_handle(FinalizablePersistentHandle::New(
-        thread_->isolate_group(), to, tpeer, FreeTransferablePeer, length,
-        /*auto_delete=*/true));
+    FinalizablePersistentHandle* finalizable_ref =
+        FinalizablePersistentHandle::New(thread_->isolate_group(), to, tpeer,
+                                         FreeTransferablePeer, length,
+                                         /*auto_delete=*/true);
+    ASSERT(finalizable_ref != nullptr);
+    tpeer->set_handle(finalizable_ref);
     fpeer->ClearData();
   }