Release VM lock before running finalizers (#15050)

We shouldn't run any ruby code with the VM lock held.

3 files changed, 36 insertions, 8 deletions

diff --git a/gc.c b/gc.c
index a8320246d0..d1e542de2c 100644
--- a/gc.c
+++ b/gc.c
@@ -290,6 +290,7 @@ rb_gc_run_obj_finalizer(VALUE objid, long count, VALUE (*callback)(long i, void
     saved.finished = 0;
     saved.final = Qundef;
 
+    ASSERT_vm_unlocking();
     rb_ractor_ignore_belonging(true);
     EC_PUSH_TAG(ec);
     enum ruby_tag_type state = EC_EXEC_TAG();
diff --git a/gc/default/default.c b/gc/default/default.c
index e0a5aade85..6045cec598 100644
--- a/gc/default/default.c
+++ b/gc/default/default.c
@@ -2763,24 +2763,27 @@ rb_gc_impl_define_finalizer(void *objspace_ptr, VALUE obj, VALUE block)
 
     RBASIC(obj)->flags |= FL_FINALIZE;
 
-    int lev = RB_GC_VM_LOCK();
+    unsigned int lev = RB_GC_VM_LOCK();
 
     if (st_lookup(finalizer_table, obj, &data)) {
         table = (VALUE)data;
+        VALUE dup_table = rb_ary_dup(table);
 
+        RB_GC_VM_UNLOCK(lev);
         /* avoid duplicate block, table is usually small */
         {
             long len = RARRAY_LEN(table);
             long i;
 
             for (i = 0; i < len; i++) {
-                VALUE recv = RARRAY_AREF(table, i);
-                if (rb_equal(recv, block)) {
-                    RB_GC_VM_UNLOCK(lev);
+                VALUE recv = RARRAY_AREF(dup_table, i);
+                if (rb_equal(recv, block)) { // can't be called with VM lock held
                     return recv;
                 }
             }
         }
+        lev = RB_GC_VM_LOCK();
+        RB_GC_GUARD(dup_table);
 
         rb_ary_push(table, block);
     }
@@ -2841,8 +2844,8 @@ get_final(long i, void *data)
     return RARRAY_AREF(table, i + 1);
 }
 
-static void
-run_final(rb_objspace_t *objspace, VALUE zombie)
+static unsigned int
+run_final(rb_objspace_t *objspace, VALUE zombie, unsigned int lev)
 {
     if (RZOMBIE(zombie)->dfree) {
         RZOMBIE(zombie)->dfree(RZOMBIE(zombie)->data);
@@ -2853,7 +2856,9 @@ run_final(rb_objspace_t *objspace, VALUE zombie)
         FL_UNSET(zombie, FL_FINALIZE);
         st_data_t table;
         if (st_delete(finalizer_table, &key, &table)) {
+            RB_GC_VM_UNLOCK(lev);
             rb_gc_run_obj_finalizer(RARRAY_AREF(table, 0), RARRAY_LEN(table) - 1, get_final, (void *)table);
+            lev = RB_GC_VM_LOCK();
         }
         else {
             rb_bug("FL_FINALIZE flag is set, but finalizers are not found");
@@ -2862,6 +2867,7 @@ run_final(rb_objspace_t *objspace, VALUE zombie)
     else {
         GC_ASSERT(!st_lookup(finalizer_table, key, NULL));
     }
+    return lev;
 }
 
 static void
@@ -2874,9 +2880,9 @@ finalize_list(rb_objspace_t *objspace, VALUE zombie)
         next_zombie = RZOMBIE(zombie)->next;
         page = GET_HEAP_PAGE(zombie);
 
-        int lev = RB_GC_VM_LOCK();
+        unsigned int lev = RB_GC_VM_LOCK();
 
-        run_final(objspace, zombie);
+        lev = run_final(objspace, zombie, lev);
         {
             GC_ASSERT(BUILTIN_TYPE(zombie) == T_ZOMBIE);
             GC_ASSERT(page->heap->final_slots_count > 0);
diff --git a/test/ruby/test_gc.rb b/test/ruby/test_gc.rb
index a8a937f078..7695fd33cf 100644
--- a/test/ruby/test_gc.rb
+++ b/test/ruby/test_gc.rb
@@ -914,4 +914,25 @@ class TestGc < Test::Unit::TestCase
       assert_include ObjectSpace.dump(young_obj), '"old":true'
     end
   end
+
+  def test_finalizer_not_run_with_vm_lock
+    assert_ractor(<<~'RUBY')
+      Thread.new do
+        loop do
+          Encoding.list.each do |enc|
+            enc.names
+          end
+        end
+      end
+
+      o = Object.new
+      ObjectSpace.define_finalizer(o, proc do
+        sleep 0.5 # finalizer shouldn't be run with VM lock, otherwise this context switch will crash
+      end)
+      o = nil
+      4.times do
+        GC.start
+      end
+    RUBY
+  end
 end