diff --git a/ext/fiddle/function.c b/ext/fiddle/function.c index dc4901bf..b0ff48e2 100644 --- a/ext/fiddle/function.c +++ b/ext/fiddle/function.c @@ -304,7 +304,18 @@ function_call(int argc, VALUE argv[], VALUE self) (func->is_variadic ? sizeof(int) * n_call_args : 0)); args.values = (void **)((char *)generic_args + sizeof(fiddle_generic) * n_call_args); - /* GC-scanned (conservatively) as part of the ALLOCV buffer */ + /* GC-scanned (conservatively) as part of the ALLOCV buffer. + * + * The conservative scan both keeps these values alive and holds them in + * place, and both properties are required. generic_args can hold a pointer + * into one of them: TYPE_CONST_STRING stores rb_string_value_cstr(&src), + * and for a to_str object that is the coerced String, which argv does not + * hold. ffi_call then runs without the GVL, where another thread can compact + * the heap. + * + * Storing these values somewhere the GC may move them instead, such as a + * Ruby Array, keeps them alive but not in place. The C function then reads + * the slot the String left. Keep them in this buffer. */ converted_args = (VALUE *)((char *)args.values + sizeof(void *) * (n_call_args + 1)); MEMZERO(converted_args, VALUE, n_call_args); diff --git a/test/fiddle/test_function.rb b/test/fiddle/test_function.rb index ee5fdf30..1ab7776f 100644 --- a/test/fiddle/test_function.rb +++ b/test/fiddle/test_function.rb @@ -179,6 +179,86 @@ def test_strcpy assert_equal("123", str.to_s) end + # A TYPE_CONST_STRING argument that comes from a to_str object is coerced inside + # Fiddle. The caller's argv then holds that object, not the String that the + # char * points into, so argv does not keep the String in place. Fiddle keeps the + # coerced String in converted_args instead, and the conservative scan of the + # ALLOCV buffer pins it there. + # + # That pin is what this test protects. A converted_args that the garbage + # collector can move, such as a Ruby Array, lets compaction move the String after + # the char * is stored. The C function then reads the slot the String left. + # + # The second argument must be neither a String nor a Pointer. Only then does + # Fiddle call Pointer.[] on it, and that allocation is what lets the garbage + # collector run while the char * is already in place. + def test_call_const_string_from_to_str_after_compaction + omit("Need CRuby") unless RUBY_ENGINE == "ruby" + omit("Need GC.auto_compact=") unless GC.respond_to?(:auto_compact=) + + length = 100 + subject_class = Class.new do + define_method(:to_str) { "Q" * length } + end + # 4096 bytes keep these bytes in a malloc'd buffer, which compaction does not + # move. A short String would hold its bytes in the object itself, and then this + # pointer could dangle and report a failure that this test is not about. + reject = "Z" + ("\0" * 4095) + # to_ptr must build a new Pointer on every call. A Pointer that already exists + # gives Fiddle nothing to allocate, the garbage collector then does not run + # while the char * is in place, and this test cannot fail. + opener_class = Class.new do + define_method(:to_ptr) { Pointer[reject] } + end + + strcspn = Function.new(@libc["strcspn"], + [TYPE_CONST_STRING, TYPE_VOIDP], + TYPE_SIZE_T) + # The subject holds no "Z", so strcspn stops at the terminator and reports the + # full length. A stale char * reports 0, because the slot the String left holds + # zero bytes, and churn content also reports 0. + + churn = [] + results = [] + auto_compact = GC.auto_compact + begin + 10.times do + # Churn of the same byte size as the subject, with half of it dropped, so + # that the subject's size pool holds sparsely occupied pages. The compactor + # only evacuates such pages. Without this the subject never moves, and then + # this test cannot fail. + churn.clear + 2000.times { churn << ("Z" * length) } + churn.each_index { |i| churn[i] = nil if i.even? } + GC.start + + # A short burst of same size allocations that this loop drops at once. + # It leaves a partly filled page in the subject's size pool. The + # compactor only moves objects out of pages that are not full, so + # without this burst the subject can land in a full page. It then never + # moves, and this test cannot fail. + 500.times { "y" * length } + + GC.auto_compact = true + GC.stress = true + begin + results << strcspn.call(subject_class.new, opener_class.new) + ensure + GC.stress = false + GC.auto_compact = auto_compact + end + end + ensure + GC.stress = false + GC.auto_compact = auto_compact + end + assert_equal([length] * 10, results) + # The intact case, checked after the loop: a call before it would prepare the + # CIF, and that preparation is part of what lets the collector run inside the + # window on the first measured call. + assert_equal(length, strcspn.call("Q" * length, opener_class.new)) + end + def call_proc(string_to_copy) buff = +"000" str = yield(buff, string_to_copy)