Test that a coerced TYPE_CONST_STRING argument stays in place - #213
Open
jeremy wants to merge 1 commit into
Open
Test that a coerced TYPE_CONST_STRING argument stays in place#213jeremy wants to merge 1 commit into
jeremy wants to merge 1 commit into
Conversation
A TYPE_CONST_STRING argument that comes from a to_str object is coerced inside Fiddle, so the caller's argv holds that object and not the String that the char * points into. Only converted_args holds the coerced String, and the conservative scan of the ALLOCV buffer is what keeps it in place while ffi_call runs without the GVL. Nothing tested that. Before ruby#205, converted_args was a Ruby Array, whose elements the GC may move, and the C function read the slot the String left. ruby#205 replaced the Array with this buffer and corrected the fault as a side effect. A later change could move these values back to storage the GC may relocate, and no test would notice. Add the missing test, and record the requirement next to the buffer.
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.
Problem
TYPE_CONST_STRINGstores a pointer into a Ruby String:A String argument is safe.
argvholds it and keeps it in place.A
to_strobject is not.rb_string_value_cstrwrites the coerced String back intosrc.argvstill holds the original object, not the String. The next loop turn overwritessrc. After that, onlyconverted_argsholds the String.The pointer must stay valid until
ffi_callreturns.ffi_callruns underrb_thread_call_without_gvl, so another thread can compact the heap during the call.converted_argsmust keep the String in place, not only alive.Master does this.
converted_argssits in theALLOCVbuffer. The GC scans that buffer conservatively, and a conservative scan pins the object.Gap
No test covers this.
Before #205,
converted_argswas a Ruby Array. The GC may move an Array's elements, so the String moved while the pointer stayed, and the C function read the vacated slot. #205 replaced the Array with the buffer and fixed the fault as a side effect. Nothing records that the buffer is required.Change
test_call_const_string_from_to_str_after_compactionintest/fiddle/test_function.rb.converted_argsinext/fiddle/function.cstating the requirement.The test calls
strcspnas[TYPE_CONST_STRING, TYPE_VOIDP] -> TYPE_SIZE_T. Arg 0 is ato_strobject returning a fresh 100-byte String. Arg 1 is ato_ptrobject, which makes Fiddle callPointer.[]; that allocation lets the GC run after the pointer is stored.The subject holds no
"Z"; the reject set is"Z". A correct call returns 100. A stale pointer returns 0, because a vacated slot holds zero bytes.Three details are load-bearing. Remove any one and the test passes on a faulty build; each has a comment:
to_ptrreturns a newPointereach time. An existingPointergives Fiddle nothing to allocate.Result
d389bbf(before #205)Three runs per tree on darwin, same result each time. Full suite on master with this change: 242 tests, 668 assertions, 0 failures, 1 error, 3 omissions. The error is
test_nogvl_pollneedingenvutil; it fails the same way without this change.Environment: ruby 3.4.10 [arm64-darwin23]; ruby 3.4.7 [aarch64-linux], glibc 2.36, libffi 3.4.4.
Note
Released versions still have the fault. I measured 1.1.6 and 1.1.8 on both platforms: a two-argument call of this shape returns the wrong result on every iteration under
GC.stresswithGC.auto_compact. Master is correct. You may want a release.I report this as latent. The trigger is the declared argument type plus a
to_strobject in place of a String. In one application that loads fiddle at boot, all 227 bundled gems put everyFiddle::Function.newbehind a Windows-only branch.