fiddle icon indicating copy to clipboard operation
fiddle copied to clipboard

`Fiddle::Pointer#to_value` is unsafe

Open uvlad7 opened this issue 10 months ago • 2 comments

Well, it's pretty self-explanatory. It returns (VALUE)(data->ptr), and ptr is obviously not marked, so can be GCed. This method is either should be removed completely (it'll still e possible to hack around with Fiddle.dlunwrap, but this at least feels hacky) or to be allowed only for pointers marked to be in fact VALUEs (mark return value type in Function constructor), and they of course should gc_mark their ptrs. Btw, there is no way to create a pointer from Ruby object, either.

uvlad7 avatar Feb 09 '25 21:02 uvlad7

Could you also share your real world problems that are related to Fiddle::Pointer#to_value?

kou avatar Feb 10 '25 01:02 kou

Yep, so I'm fiddling around load_iseq and I needed to modify it, had to use rb_iseq_load to convert modified version back to InstructionSequence

# https://stackoverflow.com/questions/30562292/how-can-i-store-and-read-a-rubyvminstructionsequence
# Retrieve Ruby Core's C-ext `iseq_load' function address
load_fn_addr = Fiddle::Handle::DEFAULT['rb_iseq_load']
# Retrieve `iseq_load' C function representation
# VALUE rb_iseq_load(VALUE data, VALUE parent, VALUE opt)
load_fn = Fiddle::Function.new(
  load_fn_addr,
  [Fiddle::TYPE_VOIDP] * 3,
  Fiddle::TYPE_VOIDP,
  need_gvl: true,
)

So I wrote it just like in the SO answer

# Make `load_fn' accessible as `iseq_load' class method
# cannot simply register it via rb_define_singleton_method as it doesn't accept `VALUE self`
define_singleton_method(:iseq_load) do |data, parent = nil, opt = nil|
  load_fn.call(Fiddle.dlwrap(data), Fiddle.dlwrap(parent), Fiddle.dlwrap(opt)).to_value
end

and noticed it might crash. I wasn't able to reproduce it this way, even with GC.stress = true, but when I tried

ptr = load_fn.call(Fiddle.dlwrap(data), Fiddle.dlwrap(parent), Fiddle.dlwrap(opt))
GC.start
ptr.to_value

I got a crash or an invalid object as a return value multiple times. Have to disable GC around the whole block

define_singleton_method(:iseq_load) do |data, parent = nil, opt = nil|
  begin
    disabled = GC.disable # returns true if was already disabled
    load_fn.call(Fiddle.dlwrap(data), Fiddle.dlwrap(parent), Fiddle.dlwrap(opt)).to_value
  ensure
    GC.enable unless disabled
  end
end

Note: I know about another problem with GC moving objects, and about Fiddle::Pinned, not used here because need to disable GC anyway and because target Ruby doesn't have moving GC.

uvlad7 avatar Feb 10 '25 09:02 uvlad7