Keep the Closure in place for libffi across GC compaction - #212
Open
jeremy wants to merge 2 commits into
Open
Conversation
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 ruby#211
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
ext/fiddle/closure.c:70
- Like
.dmark, the new.dcompactcan be invoked withptr == NULLafterclosure_free/initialize_rescuesetRTYPEDDATA_DATA(self) = NULL. Guard against NULL to avoid dereferencing it during compaction.
closure_compact(void *ptr)
{
fiddle_closure *closure = ptr;
closure->self = rb_gc_location(closure->self);
}
kou
reviewed
Aug 7, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #211.
Problem
Fiddle::Closuregives libffi theVALUEof the Closure object as user data. The trampoline casts that value back on each call.closure_data_typesets.dmark = 0and 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::BlockCallerandFiddle::Importer#bindfail in the same way.Change
libffi now receives the
fiddle_closurestruct. That memory comes fromxmallocand does not move.The struct holds the Closure
VALUEin a newselfmember:closure_markmarks the member withrb_gc_mark_movable.closure_compactupdates the member withrb_gc_location.((fiddle_closure *)x->ctx)->self.The Closure stays movable. The
ffigem uses the same method.Test
The new test is
test_call_after_compactionintest/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.
0x4The 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_markalso corrects the fault, and the diff is smaller. But it pins one object for each live Closure. This pull request usesrb_gc_mark_movableto prevent the pin. Please tell me if you prefer the smaller diff.