Skip to content

Commit

Permalink
Fix a very unlikely GC bug when using a callback block
Browse files Browse the repository at this point in the history
This could happen, when the Proc object created by rb_block_proc() is GC'ed before the call is executed.
Practically this is very unlikely unless called with GC.stress=true .

It can easily be fixed, by returning the Proc object and keeping it in a local variable while the call is executed.

When running the test suite with GC.stress=true then it failed like so:
Failures:

  1) Callback struct by value parameter
     Failure/Error:
       LibTest.testCallbackTrV(s) do |struct|
         s2[:s8] = struct[:s8]
         s2[:f32] = struct[:f32]
         s2[:s32] = struct[:s32]
       end

     NoMethodError:
       undefined method `call' for an instance of FFI::MemoryPointer
      ./spec/ffi/callback_spec.rb:312:in `testCallbackTrV'
      ./spec/ffi/callback_spec.rb:312:in `block (2 levels) in <module:CallbackSpecs>'

  2) Callback with  struct by reference argument
     Failure/Error: LibTest.testCallbackYrV(magic) { |i| v = i }

     NoMethodError:
       undefined method `call' for an instance of FFI::Pointer
      ./spec/ffi/callback_spec.rb:802:in `testCallbackYrV'
      ./spec/ffi/callback_spec.rb:802:in `block (2 levels) in <module:CallbackWithSpecs>'

  3) Callback with  struct by reference argument with nil value
     Failure/Error: LibTest.testCallbackYrV(nil) { |i| v = i }

     NoMethodError:
       undefined method `call' for an instance of CallbackWithSpecs::LibTest::S8F32S32
      ./spec/ffi/callback_spec.rb:809:in `testCallbackYrV'
      ./spec/ffi/callback_spec.rb:809:in `block (2 levels) in <module:CallbackWithSpecs>'

Finished in 100 minutes 21 seconds (files took 0.56651 seconds to load)
5014 examples, 3 failures
  • Loading branch information
larskanis committed Mar 31, 2024
1 parent 77f016f commit a7aa25e
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
18 changes: 12 additions & 6 deletions ext/ffi_c/Call.c
Expand Up @@ -86,10 +86,11 @@ static inline void* getPointer(VALUE value, int type);

static ID id_to_ptr, id_map_symbol, id_to_native;

void
VALUE
rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
FFIStorage* paramStorage, void** ffiValues,
VALUE* callbackParameters, int callbackCount, VALUE enums)
VALUE* callbackParameters, int callbackCount,
VALUE enums)
{
VALUE callbackProc = Qnil;
FFIStorage* param = &paramStorage[0];
Expand Down Expand Up @@ -327,6 +328,7 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
rb_raise(rb_eArgError, "Invalid parameter type: %d", paramType->nativeType);
}
}
return callbackProc;
}

static void *
Expand Down Expand Up @@ -362,6 +364,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
FFIStorage* params;
VALUE rbReturnValue;
rbffi_frame_t frame = { 0 };
VALUE callbackProc;

retval = alloca(MAX(fnInfo->ffi_cif.rtype->size, FFI_SIZEOF_ARG));

Expand All @@ -379,9 +382,10 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
bc->params = params;
bc->frame = &frame;

rbffi_SetupCallParams(argc, argv,
callbackProc = rbffi_SetupCallParams(argc, argv,
fnInfo->parameterCount, fnInfo->parameterTypes, params, ffiValues,
fnInfo->callbackParameters, fnInfo->callbackCount, fnInfo->rbEnums);
fnInfo->callbackParameters, fnInfo->callbackCount,
fnInfo->rbEnums);

rbffi_frame_push(&frame);
rb_rescue2(rbffi_do_blocking_call, (VALUE) bc, rbffi_save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0);
Expand All @@ -392,14 +396,16 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
ffiValues = ALLOCA_N(void *, fnInfo->parameterCount);
params = ALLOCA_N(FFIStorage, fnInfo->parameterCount);

rbffi_SetupCallParams(argc, argv,
callbackProc = rbffi_SetupCallParams(argc, argv,
fnInfo->parameterCount, fnInfo->parameterTypes, params, ffiValues,
fnInfo->callbackParameters, fnInfo->callbackCount, fnInfo->rbEnums);
fnInfo->callbackParameters, fnInfo->callbackCount,
fnInfo->rbEnums);

rbffi_frame_push(&frame);
ffi_call(&fnInfo->ffi_cif, FFI_FN(function), retval, ffiValues);
rbffi_frame_pop(&frame);
}
RB_GC_GUARD(callbackProc);

if (unlikely(!fnInfo->ignoreErrno)) {
rbffi_save_errno();
Expand Down
5 changes: 3 additions & 2 deletions ext/ffi_c/Call.h
Expand Up @@ -73,9 +73,10 @@ typedef union {

extern void rbffi_Call_Init(VALUE moduleFFI);

extern void rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
extern VALUE rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
FFIStorage* paramStorage, void** ffiValues,
VALUE* callbackParameters, int callbackCount, VALUE enums);
VALUE* callbackParameters, int callbackCount,
VALUE enums);

struct FunctionType_;
extern VALUE rbffi_CallFunction(int argc, VALUE* argv, void* function, struct FunctionType_* fnInfo);
Expand Down
7 changes: 5 additions & 2 deletions ext/ffi_c/Variadic.c
Expand Up @@ -202,6 +202,7 @@ variadic_invoke(VALUE self, VALUE parameterTypes, VALUE parameterValues)
Type** paramTypes;
VALUE* argv;
VALUE* callbackParameters;
VALUE callbackProc;
int paramCount = 0, fixedCount = 0, callbackCount = 0, i;
ffi_status ffiStatus;
rbffi_frame_t frame = { 0 };
Expand Down Expand Up @@ -290,8 +291,9 @@ variadic_invoke(VALUE self, VALUE parameterTypes, VALUE parameterValues)
rb_raise(rb_eArgError, "Unknown FFI error");
}

rbffi_SetupCallParams(paramCount, argv, -1, paramTypes, params,
ffiValues, callbackParameters, callbackCount, invoker->rbEnums);
callbackProc = rbffi_SetupCallParams(paramCount, argv, -1, paramTypes, params,
ffiValues, callbackParameters, callbackCount,
invoker->rbEnums);

rbffi_frame_push(&frame);

Expand All @@ -309,6 +311,7 @@ variadic_invoke(VALUE self, VALUE parameterTypes, VALUE parameterValues)
} else {
ffi_call(&cif, FFI_FN(invoker->function), retval, ffiValues);
}
RB_GC_GUARD(callbackProc);

rbffi_frame_pop(&frame);

Expand Down

0 comments on commit a7aa25e

Please sign in to comment.