Keep the Closure in place for libffi across GC compaction

ext/fiddle/closure.c

@@ -7,6 +7,7 @@ int ruby_thread_has_gvl_p(void); /* from internal.h */
 VALUE cFiddleClosure;
 
 typedef struct {
+    VALUE self;
     void * code;
     ffi_closure *pcl;
     ffi_cif cif;
@@ -54,12 +55,45 @@ closure_memsize(const void * ptr)
     return size;
 }
 
+/* Ruby 2.6 and earlier have no compaction, so marking the reference plainly
+ * holds it in place there. */
+#ifndef HAVE_RB_GC_MARK_MOVABLE
+# define rb_gc_mark_movable rb_gc_mark
+#endif
+
+/* Fiddle::Closure#free and initialize_rescue set the data pointer to NULL.
+ * The garbage collector does not call these functions with a NULL pointer,
+ * but this file is the only one in the extension that clears the pointer,
+ * so both functions check it. */
+static void
+closure_mark(void *ptr)
+{
+    fiddle_closure *closure = ptr;
+    if (closure) {
+        rb_gc_mark_movable(closure->self);
+    }
+}
+
+#ifdef HAVE_RB_GC_MARK_MOVABLE
+static void
+closure_compact(void *ptr)
+{
+    fiddle_closure *closure = ptr;
+    if (closure) {
+        closure->self = rb_gc_location(closure->self);
+    }
+}
+#endif
+
 const rb_data_type_t closure_data_type = {
     .wrap_struct_name = "fiddle/closure",
     .function = {
-        .dmark = 0,
+        .dmark = closure_mark,
         .dfree = dealloc,
-        .dsize = closure_memsize
+        .dsize = closure_memsize,
+#ifdef HAVE_RB_GC_MARK_MOVABLE
+        .dcompact = closure_compact,
+#endif
     },
     .flags = FIDDLE_DEFAULT_TYPED_DATA_FLAGS,
 };
@@ -76,7 +110,7 @@ with_gvl_callback(void *ptr)
 {
     struct callback_args *x = ptr;
 
-    VALUE self      = (VALUE)x->ctx;
+    VALUE self      = ((fiddle_closure *)x->ctx)->self;
     VALUE rbargs    = rb_iv_get(self, "@args");
     VALUE ctype     = rb_iv_get(self, "@ctype");
     int argc        = RARRAY_LENINT(rbargs);
@@ -288,6 +322,10 @@ initialize_body(VALUE user_data)
 
     TypedData_Get_Struct(data->self, fiddle_closure, &closure_data_type, cl);
 
+    /* libffi receives the address of this struct, which xmalloc'd memory keeps
+     * stable. The trampoline reads cl->self, so the GC must mark and relocate it. */
+    RB_OBJ_WRITE(data->self, &cl->self, data->self);
+
     cl->argv = (ffi_type **)xcalloc(argc + 1, sizeof(ffi_type *));
 
     normalized_args = rb_ary_new_capa(argc);
@@ -318,9 +356,9 @@ initialize_body(VALUE user_data)
 
 #if USE_FFI_CLOSURE_ALLOC
     result = ffi_prep_closure_loc(pcl, cif, callback,
-                                  (void *)(data->self), cl->code);
+                                  (void *)cl, cl->code);
 #else
-    result = ffi_prep_closure(pcl, cif, callback, (void *)(data->self));
+    result = ffi_prep_closure(pcl, cif, callback, (void *)cl);
     cl->code = (void *)pcl;
     i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC);
     if (i) {

ext/fiddle/extconf.rb

@@ -241,6 +241,7 @@ def enable_debug_build_flag(flags)
 end
 
 have_func("rb_str_to_interned_str")
+have_func("rb_gc_mark_movable") # RUBY_VERSION >= 2.7
 have_const("RUBY_TYPED_EMBEDDABLE", "ruby.h") # RUBY_VERSION >= 3.3
 create_makefile 'fiddle' do |conf|
   if !libffi

test/fiddle/test_closure.rb

@@ -172,6 +172,54 @@ def call
       end
     end
 
+    def test_call_after_compaction
+      unless GC.respond_to?(:verify_compaction_references)
+        omit("Need GC.verify_compaction_references")
+      end
+      omit("Need CRuby") unless RUBY_ENGINE == "ruby"
+      require "envutil" unless defined?(EnvUtil)
+
+      # A separate process, because expand_heap: doubles the heap and the pages
+      # stay. Expanding this process's heap changes how often the collector runs
+      # afterwards, and test_no_memory_leak reads that as growth.
+      script = <<~'RUBY'
+        require "fiddle"
+
+        closure_class = Class.new(Fiddle::Closure) do
+          def call(a, b)
+            a + b
+          end
+        end
+
+        # The closure must be reachable only through the heap. A local variable is
+        # pinned by the conservative machine-stack scan. A pinned closure does not
+        # move, and then this test cannot fail.
+        holder = [closure_class.new(Fiddle::TYPE_INT,
+                                    [Fiddle::TYPE_INT, Fiddle::TYPE_INT])]
+        begin
+          begin
+            GC.verify_compaction_references(expand_heap: true, toward: :empty)
+          rescue ArgumentError
+            # Ruby 3.1 and earlier spell expand_heap: as double_heap:
+            GC.verify_compaction_references(double_heap: true, toward: :empty)
+          end
+        rescue NotImplementedError
+          # A Ruby without compaction cannot move the closure. The call below
+          # still exercises the plain path.
+        end
+        func = Fiddle::Function.new(holder[0].to_i,
+                                    [Fiddle::TYPE_INT, Fiddle::TYPE_INT],
+                                    Fiddle::TYPE_INT)
+        puts(func.call(40, 2))
+        holder[0].free
+      RUBY
+      load_path_args = $LOAD_PATH.flat_map {|path| ["-I", path]}
+      stdout, stderr, status = EnvUtil.invoke_ruby([*load_path_args, "-e", script],
+                                                   "", true, true)
+      assert(status.success?, stderr)
+      assert_equal("42", stdout.chomp)
+    end
+
     def test_ractor_shareable
       omit("Need Ractor") unless defined?(Ractor)
       closure_class = Class.new(Closure) do