From 9cdce0277f564b45cc5c5a07619b72edec8824af Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 18:14:37 -0700 Subject: [PATCH 1/2] Keep the Closure in place for libffi across GC compaction Fiddle::Closure gave libffi the VALUE of the Closure object as user data. The trampoline cast that value back on each call. closure_data_type set .dmark = 0 and declared no .dcompact, so nothing kept the object in place. When GC compaction moved the Closure, libffi returned a dead address and the process stopped with SIGSEGV. Closure::BlockCaller and Importer#bind failed in the same way. libffi now receives the fiddle_closure struct. That memory comes from xmalloc and does not move. The struct holds the Closure VALUE in a new self member. closure_mark marks the member with rb_gc_mark_movable and closure_compact updates it with rb_gc_location, so the Closure stays movable. The ffi gem uses the same method. The new test keeps the Closure in an array. A local variable is pinned by the conservative machine-stack scan, and a pinned Closure does not move. The test then cannot fail, so the comment records the reason. Before this change the test stops with SIGSEGV at 0x4. After it, the test passes and the full suite reports 242 tests, 670 assertions, 0 failures, 0 errors and 3 omissions on ruby 4.0.6 (arm64-darwin23). Fixes https://github.com/ruby/fiddle/issues/211 --- ext/fiddle/closure.c | 30 +++++++++++++++++++++++++----- test/fiddle/test_closure.rb | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/ext/fiddle/closure.c b/ext/fiddle/closure.c index 5d936473..c1dcc2e8 100644 --- a/ext/fiddle/closure.c +++ b/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,27 @@ closure_memsize(const void * ptr) return size; } +static void +closure_mark(void *ptr) +{ + fiddle_closure *closure = ptr; + rb_gc_mark_movable(closure->self); +} + +static void +closure_compact(void *ptr) +{ + fiddle_closure *closure = ptr; + closure->self = rb_gc_location(closure->self); +} + 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, + .dcompact = closure_compact }, .flags = FIDDLE_DEFAULT_TYPED_DATA_FLAGS, }; @@ -76,7 +92,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 +304,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 +338,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) { diff --git a/test/fiddle/test_closure.rb b/test/fiddle/test_closure.rb index ee9ef044..fdaf02a1 100644 --- a/test/fiddle/test_closure.rb +++ b/test/fiddle/test_closure.rb @@ -172,6 +172,30 @@ def call end end + def test_call_after_compaction + omit("Need GC.verify_compaction_references") unless + GC.respond_to?(:verify_compaction_references) + omit("Need CRuby") unless RUBY_ENGINE == "ruby" + + closure_class = Class.new(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(TYPE_INT, [TYPE_INT, TYPE_INT])] + begin + GC.verify_compaction_references(expand_heap: true, toward: :empty) + func = Function.new(holder[0].to_i, [TYPE_INT, TYPE_INT], TYPE_INT) + assert_equal(42, func.call(40, 2)) + ensure + holder[0].free + end + end + def test_ractor_shareable omit("Need Ractor") unless defined?(Ractor) closure_class = Class.new(Closure) do From f695b2628c5764708a97b414628c90ced70613e1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 20:34:23 -0700 Subject: [PATCH 2/2] Check the data pointer in the Closure mark and compact functions Fiddle::Closure#free and initialize_rescue set the data pointer to NULL. The garbage collector does not call a mark function with a NULL data pointer, so this is not a defect that a test can show. But closure.c is the only file in the extension that clears the pointer, so both new functions now check it. Also change the test to a block form of unless, as the review asks. --- ext/fiddle/closure.c | 12 ++++++++++-- test/fiddle/test_closure.rb | 5 +++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/ext/fiddle/closure.c b/ext/fiddle/closure.c index c1dcc2e8..45713ffd 100644 --- a/ext/fiddle/closure.c +++ b/ext/fiddle/closure.c @@ -55,18 +55,26 @@ closure_memsize(const void * ptr) return size; } +/* 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; - rb_gc_mark_movable(closure->self); + if (closure) { + rb_gc_mark_movable(closure->self); + } } static void closure_compact(void *ptr) { fiddle_closure *closure = ptr; - closure->self = rb_gc_location(closure->self); + if (closure) { + closure->self = rb_gc_location(closure->self); + } } const rb_data_type_t closure_data_type = { diff --git a/test/fiddle/test_closure.rb b/test/fiddle/test_closure.rb index fdaf02a1..59a92015 100644 --- a/test/fiddle/test_closure.rb +++ b/test/fiddle/test_closure.rb @@ -173,8 +173,9 @@ def call end def test_call_after_compaction - omit("Need GC.verify_compaction_references") unless - GC.respond_to?(:verify_compaction_references) + unless GC.respond_to?(:verify_compaction_references) + omit("Need GC.verify_compaction_references") + end omit("Need CRuby") unless RUBY_ENGINE == "ruby" closure_class = Class.new(Closure) do