Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Reject callback with :string return type #782

Merged
merged 1 commit into from May 27, 2020

Conversation

larskanis
Copy link
Member

Callbacks returning a :string type were not supported so far. It was possible to define such a callback, but the value returned was NULL in any case. This implementation rejects :string return type of callbacks.

The reason for the reject is the following: In contrast to :string parameters to called functions there is no well defined ownership of the memory of the string returned by callbacks. Instead an explicit FFI::MemoryPointer or similar should be used, which allows to track the validity of underlying memory instead of relying on some Ruby implementation details.

Fixes #751
Alternative to #770

@larskanis larskanis requested review from eregon and headius and removed request for headius and eregon May 23, 2020 21:38
@larskanis larskanis changed the title Reject callback with :string return type [WIP] Reject callback with :string return type May 23, 2020
@eregon
Copy link
Collaborator

eregon commented May 23, 2020

The check in this PR doesn't seem correct because it prevents:

attach_function :ptr_ret_pointer, [ :pointer, :int], :string

and so tests fail.

What's the difference between a function returning a string and a callback returning a string? (it's late, I might miss something obvious)

@eregon
Copy link
Collaborator

eregon commented May 23, 2020

What's the difference between a function returning a string and a callback returning a string? (it's late, I might miss something obvious)

Got it. If it's just a normal call, we can just read the C String to a Ruby String with

FFI::Pointer.new(address).read_string_to_null

and not have to worry what's the lifetime of that pointer.
(Well, since FFI doesn't return the pointer, there better be a separate way to free it)

But for a callback, we have a Proc returning a Ruby String, and we need to give it as char* which is indeed is quite brittle, if e.g. the Ruby String is not kept alive, or the MRI GC decides to move the String pointer. Maybe RSTRING_PTR pins the object so it can actually not move after RSTRING_PTR?

Callbacks returning a :string type were not supported so far and did not work.
It was possible to define such a callback, but the value returned was NULL in any case.
This implementation rejects :string return type of callbacks at the definition.

The reason for the reject is the following:
In contrast to :string parameters to called functions there is no well defined ownership of the memory of the string returned by callbacks.
Instead an explicit FFI::MemoryPointer or similar should be used, which allows to track the validity of underlying memory instead of relying on some Ruby implementation details.

Fixes ffi#751
Copy link
Collaborator

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me and is safe.
And we can always evolve into allowing more (e.g., something based on strdup), but not the other way around for compatibility.

@larskanis
Copy link
Member Author

@eregon Thank you for looking into the PR! I'm still busy in teaching my children as a second job due to closed schools due to the corona lockdown since 10 weeks. That's why my responses are so sparse.

@larskanis larskanis merged commit d510675 into ffi:master May 27, 2020
graalvmbot pushed a commit to oracle/truffleruby that referenced this pull request Jun 5, 2020
* Applies ffi/ffi#782
* So the semantics of a callback returning :string is well defined.
@eregon
Copy link
Collaborator

eregon commented Jun 8, 2020

FYI: I applied the same change in TruffleRuby master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants