Skip to content

Commit 195c8d1

Browse files
authored
Keep the Closure in place for libffi across GC compaction (#212)
Closes #211. ## Problem `Fiddle::Closure` gives libffi the `VALUE` of the Closure object as user data. The trampoline casts that value back on each call. `closure_data_type` sets `.dmark = 0` and declares no `.dcompact`. Nothing keeps the object in place. GC compaction can move the Closure. After a move, libffi gives back a dead address, and the process stops with SIGSEGV. `Fiddle::Closure::BlockCaller` and `Fiddle::Importer#bind` fail in the same way. ## Change 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`. - `closure_compact` updates the member with `rb_gc_location`. - The trampoline reads `((fiddle_closure *)x->ctx)->self`. The Closure stays movable. The `ffi` gem uses the same method. ## Test The new test is `test_call_after_compaction` in `test/fiddle/test_closure.rb`. The test keeps the Closure in an array. A local variable is pinned by the conservative machine-stack scan. A pinned Closure does not move, and then the test cannot fail. A comment in the test records this reason. | build | result | |---|---| | before this change | SIGSEGV at `0x4` | | after this change | pass | The full suite after this change: 242 tests, 670 assertions, 0 failures, 0 errors, 3 omissions. Environment: ruby 4.0.6 (2026-07-14 revision 03b6d3f889) +PRISM [arm64-darwin23]. ## Alternative `rb_gc_mark` also corrects the fault, and the diff is smaller. But it pins one object for each live Closure. This pull request uses `rb_gc_mark_movable` to prevent the pin. Please tell me if you prefer the smaller diff.
1 parent e6c49f9 commit 195c8d1

3 files changed

Lines changed: 84 additions & 5 deletions

File tree

‎ext/fiddle/closure.c‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ int ruby_thread_has_gvl_p(void); /* from internal.h */
77
VALUE cFiddleClosure;
88

99
typedef struct {
10+
VALUE self;
1011
void * code;
1112
ffi_closure *pcl;
1213
ffi_cif cif;
@@ -54,12 +55,37 @@ closure_memsize(const void * ptr)
5455
return size;
5556
}
5657

58+
/* Ruby 2.6 and earlier have no compaction, so marking the reference plainly
59+
* holds it in place there. */
60+
#ifndef HAVE_RB_GC_MARK_MOVABLE
61+
# define rb_gc_mark_movable rb_gc_mark
62+
#endif
63+
64+
static void
65+
closure_mark(void *ptr)
66+
{
67+
fiddle_closure *closure = ptr;
68+
rb_gc_mark_movable(closure->self);
69+
}
70+
71+
#ifdef HAVE_RB_GC_MARK_MOVABLE
72+
static void
73+
closure_compact(void *ptr)
74+
{
75+
fiddle_closure *closure = ptr;
76+
closure->self = rb_gc_location(closure->self);
77+
}
78+
#endif
79+
5780
const rb_data_type_t closure_data_type = {
5881
.wrap_struct_name = "fiddle/closure",
5982
.function = {
60-
.dmark = 0,
83+
.dmark = closure_mark,
6184
.dfree = dealloc,
62-
.dsize = closure_memsize
85+
.dsize = closure_memsize,
86+
#ifdef HAVE_RB_GC_MARK_MOVABLE
87+
.dcompact = closure_compact,
88+
#endif
6389
},
6490
.flags = FIDDLE_DEFAULT_TYPED_DATA_FLAGS,
6591
};
@@ -76,7 +102,7 @@ with_gvl_callback(void *ptr)
76102
{
77103
struct callback_args *x = ptr;
78104

79-
VALUE self = (VALUE)x->ctx;
105+
VALUE self = ((fiddle_closure *)x->ctx)->self;
80106
VALUE rbargs = rb_iv_get(self, "@args");
81107
VALUE ctype = rb_iv_get(self, "@ctype");
82108
int argc = RARRAY_LENINT(rbargs);
@@ -288,6 +314,10 @@ initialize_body(VALUE user_data)
288314

289315
TypedData_Get_Struct(data->self, fiddle_closure, &closure_data_type, cl);
290316

317+
/* libffi receives the address of this struct, which xmalloc'd memory keeps
318+
* stable. The trampoline reads cl->self, so the GC must mark and relocate it. */
319+
RB_OBJ_WRITE(data->self, &cl->self, data->self);
320+
291321
cl->argv = (ffi_type **)xcalloc(argc + 1, sizeof(ffi_type *));
292322

293323
normalized_args = rb_ary_new_capa(argc);
@@ -318,9 +348,9 @@ initialize_body(VALUE user_data)
318348

319349
#if USE_FFI_CLOSURE_ALLOC
320350
result = ffi_prep_closure_loc(pcl, cif, callback,
321-
(void *)(data->self), cl->code);
351+
(void *)cl, cl->code);
322352
#else
323-
result = ffi_prep_closure(pcl, cif, callback, (void *)(data->self));
353+
result = ffi_prep_closure(pcl, cif, callback, (void *)cl);
324354
cl->code = (void *)pcl;
325355
i = mprotect(pcl, sizeof(*pcl), PROT_READ | PROT_EXEC);
326356
if (i) {

‎ext/fiddle/extconf.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,7 @@ def enable_debug_build_flag(flags)
241241
end
242242

243243
have_func("rb_str_to_interned_str")
244+
have_func("rb_gc_mark_movable") # RUBY_VERSION >= 2.7
244245
have_const("RUBY_TYPED_EMBEDDABLE", "ruby.h") # RUBY_VERSION >= 3.3
245246
create_makefile 'fiddle' do |conf|
246247
if !libffi

‎test/fiddle/test_closure.rb‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,54 @@ def call
172172
end
173173
end
174174

175+
def test_call_after_compaction
176+
unless GC.respond_to?(:verify_compaction_references)
177+
omit("Need GC.verify_compaction_references")
178+
end
179+
omit("Need CRuby") unless RUBY_ENGINE == "ruby"
180+
require "envutil" unless defined?(EnvUtil)
181+
182+
# A separate process, because expand_heap: doubles the heap and the pages
183+
# stay. Expanding this process's heap changes how often the collector runs
184+
# afterwards, and test_no_memory_leak reads that as growth.
185+
script = <<~'RUBY'
186+
require "fiddle"
187+
188+
closure_class = Class.new(Fiddle::Closure) do
189+
def call(a, b)
190+
a + b
191+
end
192+
end
193+
194+
# The closure must be reachable only through the heap. A local variable is
195+
# pinned by the conservative machine-stack scan. A pinned closure does not
196+
# move, and then this test cannot fail.
197+
holder = [closure_class.new(Fiddle::TYPE_INT,
198+
[Fiddle::TYPE_INT, Fiddle::TYPE_INT])]
199+
begin
200+
begin
201+
GC.verify_compaction_references(expand_heap: true, toward: :empty)
202+
rescue ArgumentError
203+
# Ruby 3.1 and earlier spell expand_heap: as double_heap:
204+
GC.verify_compaction_references(double_heap: true, toward: :empty)
205+
end
206+
rescue NotImplementedError
207+
# A Ruby without compaction cannot move the closure. The call below
208+
# still exercises the plain path.
209+
end
210+
func = Fiddle::Function.new(holder[0].to_i,
211+
[Fiddle::TYPE_INT, Fiddle::TYPE_INT],
212+
Fiddle::TYPE_INT)
213+
puts(func.call(40, 2))
214+
holder[0].free
215+
RUBY
216+
load_path_args = $LOAD_PATH.flat_map {|path| ["-I", path]}
217+
stdout, stderr, status = EnvUtil.invoke_ruby([*load_path_args, "-e", script],
218+
"", true, true)
219+
assert(status.success?, stderr)
220+
assert_equal("42", stdout.chomp)
221+
end
222+
175223
def test_ractor_shareable
176224
omit("Need Ractor") unless defined?(Ractor)
177225
closure_class = Class.new(Closure) do

0 commit comments

Comments
 (0)