Fix use after free bug in actor heap finalisation that can lead to a …

…segfault (#4522)

A long time ago, some rando named "Dipin Hora" decided to be clever and rework how objects with finalisers are stored/garbage collected in actor heaps (see: #1638). Unfortunately for us all, he wasn't nearly as clever as he thought and introduced a use after free bug that only occurs when finalisers have logic to reference other objects that might have already been freed and reused during the same garbage collection process. Luckily for us, a smarterer rando who also happens to be named "Dipin Hora" has come along to save the day.

This commit adds in tests to reproduce the bug and rework the actor heap garbage collection logic to make sure this issue can no longer occur by making sure that:

* finalisers can no longer re-use memory that might be freed earlier in the garbage collection process
* heap chunks are only freed/destroyed after all finalisers have been run to ensure all cross object references in finalisers remain valid at the time the finalisers are run

.release-notes/4522.md

@@ -0,0 +1,3 @@
+## Fix use after free bug in actor heap finalisation that can lead to a segfault
+
+The [0.45.2](https://github.com/ponylang/ponyc/releases/tag/0.45.2) release introduced an improvement to handling of objects with finalisers to make them more efficient to allocate on actor heaps. However, in the process it also introduced a potential for use after free situations that could lead to segfaults when running finalisers. With this change, we've reworked the implementation of the actor heap garbage collection to avoid the potential for use after free situations entirely.

src/libponyrt/mem/heap.c

@@ -324,6 +324,9 @@ static void destroy_small(small_chunk_t* chunk, uint32_t mark)
 
   char* m = get_m((chunk_t*)chunk);
   ponyint_pagemap_set(m, NULL, NULL);
+#ifndef PONY_NDEBUG
+  memset(m, 0, sizeof(block_t));
+#endif
   POOL_FREE(block_t, m);
   POOL_FREE(small_chunk_t, chunk);
 }
@@ -340,12 +343,17 @@ static void destroy_large(large_chunk_t* chunk, uint32_t mark)
   large_pagemap(m, chunk->size, NULL, NULL);
 
   if(m != NULL)
+  {
+#ifndef PONY_NDEBUG
+    memset(m, 0, chunk->size);
+#endif
     ponyint_pool_free_size(chunk->size, m);
+  }
 
   POOL_FREE(large_chunk_t, chunk);
 }
 
-static size_t sweep_small(small_chunk_t* chunk, small_chunk_t** avail, small_chunk_t** full,
+static size_t sweep_small(small_chunk_t* chunk, small_chunk_t** avail, small_chunk_t** full, small_chunk_t** to_destroy,
 #ifdef USE_RUNTIMESTATS
   uint32_t empty, size_t size, size_t* mem_allocated, size_t* mem_used,
   size_t* num_allocated)
@@ -383,7 +391,13 @@ static size_t sweep_small(small_chunk_t* chunk, small_chunk_t** avail, small_chu
       chunk->next = *full;
       *full = chunk;
     } else if(chunk->slots == empty) {
-      destroy_small(chunk, 0);
+      // run finalisers for freed slots
+      final_small(chunk, FORCE_ALL_FINALISERS);
+
+      // cannot destroy chunk yet since the heap has not been fully swept yet
+      // and another object may still reference this chunk and access it in a finaliser
+      chunk->next = *to_destroy;
+      *to_destroy = chunk;
     } else {
 #ifdef USE_RUNTIMESTATS
       *mem_allocated += POOL_ALLOC_SIZE(small_chunk_t);
@@ -398,9 +412,6 @@ static size_t sweep_small(small_chunk_t* chunk, small_chunk_t** avail, small_chu
       // run finalisers for freed slots
       final_small(chunk, FORCE_NO_FINALISERS);
 
-      // make chunk available for allocations only after finalisers have been
-      // run to prevent premature reuse of memory slots by an allocation
-      // required for finaliser execution
       chunk->next = *avail;
       *avail = chunk;
     }
@@ -416,10 +427,10 @@ static size_t sweep_small(small_chunk_t* chunk, small_chunk_t** avail, small_chu
 }
 
 #ifdef USE_RUNTIMESTATS
-static large_chunk_t* sweep_large(large_chunk_t* chunk, size_t* used, size_t* mem_allocated,
+static large_chunk_t* sweep_large(large_chunk_t* chunk, large_chunk_t** to_destroy, size_t* used, size_t* mem_allocated,
   size_t* mem_used, size_t* num_allocated)
 #else
-static large_chunk_t* sweep_large(large_chunk_t* chunk, size_t* used)
+static large_chunk_t* sweep_large(large_chunk_t* chunk, large_chunk_t** to_destroy, size_t* used)
 #endif
 {
 #ifdef USE_RUNTIMESTATS
@@ -452,7 +463,13 @@ static large_chunk_t* sweep_large(large_chunk_t* chunk, size_t* used)
 #endif
       *used += chunk->size;
     } else {
-      destroy_large(chunk, 0);
+      // run finaliser
+      final_large(chunk, 0);
+
+      // cannot destroy chunk yet since the heap has not been fully swept yet
+      // and another object may still reference this chunk and access it in a finaliser
+      chunk->next = *to_destroy;
+      *to_destroy = chunk;
     }
 
     chunk = next;
@@ -854,38 +871,124 @@ void ponyint_heap_endgc(heap_t* heap
   size_t num_allocated = 0;
 #endif
 
+  // make a copy of all the heap list pointers into a temporary heap
+  heap_t theap = {};
+  heap_t* temp_heap = &theap;
+  memcpy(temp_heap, heap, offsetof(heap_t, used));
+
+  // set all the heap list pointers to NULL in the real heap to ensure that
+  // any new allocations by finalisers during the GC process don't reuse memory
+  // freed during the GC process
+  memset(heap, 0, offsetof(heap_t, used));
+
+  // lists of chunks to destroy
+  small_chunk_t* to_destroy_small = NULL;
+  large_chunk_t* to_destroy_large = NULL;
+
+  // sweep all the chunks in the temporary heap while accumulating chunks to destroy
+  // NOTE: the real heap will get new chunks allocated and added to it during this process
+  //       if a finaliser allocates memory... we have to make sure that finalisers don't
+  //       reuse memory being freed or we'll end up with a use-after-free bug due to
+  //       potential cross objects references in finalisers after memory has been reused.
   for(int i = 0; i < HEAP_SIZECLASSES; i++)
   {
-    small_chunk_t* list1 = heap->small_free[i];
-    small_chunk_t* list2 = heap->small_full[i];
+    small_chunk_t* list1 = temp_heap->small_free[i];
+    small_chunk_t* list2 = temp_heap->small_full[i];
 
-    heap->small_free[i] = NULL;
-    heap->small_full[i] = NULL;
+    temp_heap->small_free[i] = NULL;
+    temp_heap->small_full[i] = NULL;
 
-    small_chunk_t** avail = &heap->small_free[i];
-    small_chunk_t** full = &heap->small_full[i];
+    small_chunk_t** avail = &temp_heap->small_free[i];
+    small_chunk_t** full = &temp_heap->small_full[i];
 
     size_t size = SIZECLASS_SIZE(i);
     uint32_t empty = sizeclass_empty[i];
 
 #ifdef USE_RUNTIMESTATS
-    used += sweep_small(list1, avail, full, empty, size,
+    used += sweep_small(list1, avail, full, &to_destroy_small, empty, size,
       &mem_allocated, &mem_used, &num_allocated);
-    used += sweep_small(list2, avail, full, empty, size,
+    used += sweep_small(list2, avail, full, &to_destroy_small, empty, size,
       &mem_allocated, &mem_used, &num_allocated);
 #else
-    used += sweep_small(list1, avail, full, empty, size);
-    used += sweep_small(list2, avail, full, empty, size);
+    used += sweep_small(list1, avail, full, &to_destroy_small, empty, size);
+    used += sweep_small(list2, avail, full, &to_destroy_small, empty, size);
 #endif
   }
 
 #ifdef USE_RUNTIMESTATS
-  heap->large = sweep_large(heap->large, &used, &mem_allocated, &mem_used,
+  temp_heap->large = sweep_large(temp_heap->large, &to_destroy_large, &used, &mem_allocated, &mem_used,
     &num_allocated);
 #else
-  heap->large = sweep_large(heap->large, &used);
+  temp_heap->large = sweep_large(temp_heap->large, &to_destroy_large, &used);
 #endif
 
+  // we need to combine the temporary heap lists with the real heap lists
+  // because the real heap might not be empty due to allocations in finalisers
+  // any new chunks with no finalisers to run can be destroyed immediately
+  // the new allocations will get GC'd in the next GC cycle even though they
+  // are definitely not reachable by the actor anymore
+  for(int i = 0; i < HEAP_SIZECLASSES; i++)
+  {
+    // iterate to the end of the real heap list and append the temporary heap list
+    // because the real heap list is likely to be smaller than the temporary heap list
+    // and remove any chunks that have no finalisers to run immediately
+    small_chunk_t** sc_temp = &heap->small_free[i];
+    while (*sc_temp != NULL)
+    {
+      if ((*sc_temp)->finalisers == 0)
+      {
+        small_chunk_t* temp = *sc_temp;
+        *sc_temp = temp->next;
+        temp->next = to_destroy_small;
+        to_destroy_small = temp;
+      } else {
+        sc_temp = &(*sc_temp)->next;
+      }
+    }
+    *sc_temp = temp_heap->small_free[i];
+
+    // iterate to the end of the real heap list and append the temporary heap list
+    // because the real heap list is likely to be smaller than the temporary heap list
+    // and remove any chunks that have no finalisers to run immediately
+    sc_temp = &heap->small_full[i];
+    while (*sc_temp != NULL)
+    {
+      if ((*sc_temp)->finalisers == 0)
+      {
+        small_chunk_t* temp = *sc_temp;
+        *sc_temp = temp->next;
+        temp->next = to_destroy_small;
+        to_destroy_small = temp;
+      } else {
+        sc_temp = &(*sc_temp)->next;
+      }
+    }
+    *sc_temp = temp_heap->small_full[i];
+  }
+
+  // iterate to the end of the real heap list and append the temporary heap list
+  // because the real heap list is likely to be smaller than the temporary heap list
+  // and remove any chunks that have no finalisers to run immediately
+  large_chunk_t** lc_temp = &heap->large;
+  while (*lc_temp != NULL)
+  {
+    if (get_large_chunk_finaliser(*lc_temp) == 0)
+    {
+      large_chunk_t* temp = *lc_temp;
+      *lc_temp = temp->next;
+      temp->next = to_destroy_large;
+      to_destroy_large = temp;
+    } else {
+      lc_temp = &(*lc_temp)->next;
+    }
+  }
+  *lc_temp = temp_heap->large;
+
+  // destroy all the chunks that need to be destroyed
+  small_chunk_list(destroy_small, to_destroy_small, 0);
+  large_chunk_list(destroy_large, to_destroy_large, 0);
+
+
   // Foreign object sizes will have been added to heap->used already. Here we
   // add local object sizes as well and set the next gc point for when memory
   // usage has increased.

test/full-program-tests/large-dependent-finalisers/additional.cc

@@ -0,0 +1,29 @@
+#include <stdint.h>
+#include <pony.h>
+
+#ifdef _MSC_VER
+#  define EXPORT_SYMBOL __declspec(dllexport)
+#else
+#  define EXPORT_SYMBOL
+#endif
+
+extern "C"
+{
+
+static uint32_t num_objects = 3;
+
+EXPORT_SYMBOL extern void codegentest_large_dependent_finalisers_decrement_num_objects()
+{
+  num_objects--;
+  pony_exitcode((int)num_objects);
+}
+
+#ifndef _MSC_VER
+void Main_runtime_override_defaults_oo(void* opt)
+{
+  (void)opt;
+  return;
+}
+#endif
+
+}

test/full-program-tests/large-dependent-finalisers/expected-exit-code.txt

@@ -0,0 +1 @@
+1

test/full-program-tests/large-dependent-finalisers/main.pony

@@ -0,0 +1,95 @@
+use "lib:large-dependent-finalisers-additional"
+
+use @codegentest_large_dependent_finalisers_decrement_num_objects[None]()
+
+class _Final
+  let num: USize
+  let num2: USize = 0
+  let num3: USize = 0
+  let num4: USize = 0
+  let num5: USize = 0
+  let num6: USize = 0
+  let num7: USize = 0
+  let num8: USize = 0
+  let num9: USize = 0
+  let num10: USize = 0
+  let num11: USize = 0
+  let num12: USize = 0
+  let num13: USize = 0
+  let num14: USize = 0
+  let num15: USize = 0
+  let num16: USize = 0
+  let num17: USize = 0
+  let num18: USize = 0
+  let num19: USize = 0
+  let num20: USize = 0
+  let num21: USize = 0
+  let num22: USize = 0
+  let num23: USize = 0
+  let num24: USize = 0
+  let num25: USize = 0
+  let num26: USize = 0
+  let num27: USize = 0
+  let num28: USize = 0
+  let num29: USize = 0
+  let num30: USize = 0
+  let num31: USize = 0
+  let num32: USize = 0
+  let num33: USize = 0
+  let num34: USize = 0
+  let num35: USize = 0
+  let num36: USize = 0
+  let num37: USize = 0
+  let num38: USize = 0
+  let num39: USize = 0
+  let num40: USize = 0
+  let num41: USize = 0
+  let num42: USize = 0
+  let num43: USize = 0
+  let num44: USize = 0
+  let num45: USize = 0
+  let num46: USize = 0
+  let num47: USize = 0
+  let num48: USize = 0
+  let num49: USize = 0
+  let num50: USize = 0
+  let num51: USize = 0
+  let num52: USize = 0
+  let num53: USize = 0
+  let num54: USize = 0
+  let num55: USize = 0
+  let num56: USize = 0
+  let num57: USize = 0
+  let num58: USize = 0
+  let num59: USize = 0
+  let num60: USize = 0
+  let num61: USize = 0
+  let num62: USize = 0
+  let num63: USize = 0
+  let num64: USize = 0
+  let num65: USize = 0
+  let num66: USize = 0
+  let num67: USize = 0
+  let num68: USize = 0
+  let num69: USize = 0
+  let num70: USize = 0
+  var other: (_Final | None) = None
+
+  new create(n: USize) =>
+    num = n
+
+  fun _final() =>
+    match other
+    | None => None
+    | let o: this->_Final => 
+      if 1025 == (num + o.num) then
+        @codegentest_large_dependent_finalisers_decrement_num_objects()
+      end
+    end  
+
+actor Main
+  new create(env: Env) =>
+    let f1 = _Final(1)
+    let f2 = _Final(1024)
+    f1.other = f2
+    f2.other = f1

test/full-program-tests/small-dependent-finalisers/additional.cc

@@ -0,0 +1,29 @@
+#include <stdint.h>
+#include <pony.h>
+
+#ifdef _MSC_VER
+#  define EXPORT_SYMBOL __declspec(dllexport)
+#else
+#  define EXPORT_SYMBOL
+#endif
+
+extern "C"
+{
+
+static uint32_t num_objects = 1025;
+
+EXPORT_SYMBOL extern void codegentest_small_dependent_finalisers_decrement_num_objects()
+{
+  num_objects--;
+  pony_exitcode((int)num_objects);
+}
+
+#ifndef _MSC_VER
+void Main_runtime_override_defaults_oo(void* opt)
+{
+  (void)opt;
+  return;
+}
+#endif
+
+}

test/full-program-tests/small-dependent-finalisers/expected-exit-code.txt

@@ -0,0 +1 @@
+1