From 9cdce0277f564b45cc5c5a07619b72edec8824af Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 6 Aug 2026 18:14:37 -0700 Subject: [PATCH 1/3] 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/3] 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 From 3ed7da2728cef61aa18f90e44b2b9eaa710a4a18 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Wed, 12 Aug 2026 13:50:22 -0700 Subject: [PATCH 3/3] Fix CI failures across Ruby versions and platforms Three separate failures: Ruby 2.6 and earlier have no rb_gc_mark_movable, rb_gc_location, or dcompact field, so closure.c did not compile. Detect rb_gc_mark_movable in extconf. Without it there is no compaction either, so plain marking holds the reference in place and the compact callback can be omitted. Ruby 3.1 and earlier spell GC.verify_compaction_references's expand_heap: keyword as double_heap:, so the new test raised ArgumentError there. Rescue and retry with the old spelling, and treat a Ruby without compaction support as nothing to provoke. expand_heap: doubles the heap, and the pages stay. That changes how often the collector runs for the rest of the suite, and test_no_memory_leak read the extra growth inside its measured block as a leak on Linux. Run the compaction scenario in a separate process, so the expansion dies with it. The subprocess still reports the fault: the child dies with SIGSEGV when closure.c stores the closure address without the mark and compact callbacks. --- ext/fiddle/closure.c | 12 ++++++++- ext/fiddle/extconf.rb | 1 + test/fiddle/test_closure.rb | 51 +++++++++++++++++++++++++++---------- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/ext/fiddle/closure.c b/ext/fiddle/closure.c index 45713ffd..8670d79e 100644 --- a/ext/fiddle/closure.c +++ b/ext/fiddle/closure.c @@ -55,6 +55,12 @@ 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, @@ -68,6 +74,7 @@ closure_mark(void *ptr) } } +#ifdef HAVE_RB_GC_MARK_MOVABLE static void closure_compact(void *ptr) { @@ -76,6 +83,7 @@ closure_compact(void *ptr) closure->self = rb_gc_location(closure->self); } } +#endif const rb_data_type_t closure_data_type = { .wrap_struct_name = "fiddle/closure", @@ -83,7 +91,9 @@ const rb_data_type_t closure_data_type = { .dmark = closure_mark, .dfree = dealloc, .dsize = closure_memsize, - .dcompact = closure_compact +#ifdef HAVE_RB_GC_MARK_MOVABLE + .dcompact = closure_compact, +#endif }, .flags = FIDDLE_DEFAULT_TYPED_DATA_FLAGS, }; diff --git a/ext/fiddle/extconf.rb b/ext/fiddle/extconf.rb index 34cd0c3d..e9fd2aa0 100644 --- a/ext/fiddle/extconf.rb +++ b/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 diff --git a/test/fiddle/test_closure.rb b/test/fiddle/test_closure.rb index 59a92015..1a37b6c0 100644 --- a/test/fiddle/test_closure.rb +++ b/test/fiddle/test_closure.rb @@ -177,24 +177,47 @@ def test_call_after_compaction omit("Need GC.verify_compaction_references") end omit("Need CRuby") unless RUBY_ENGINE == "ruby" + require "envutil" unless defined?(EnvUtil) - closure_class = Class.new(Closure) do - def call(a, b) - a + b + # 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 - 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 + # 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 - end + 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