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

Make string return value from callback available #751

Closed
wants to merge 4 commits into from

Conversation

datibbaw
Copy link

@datibbaw datibbaw commented Mar 16, 2020

Background

When a callback is defined, the return value isn't made available to the underlying C code if the return value is a string memory pointer; instead, it would return null to the caller.

Disclaimer

Tested with two return values:

  • FFI::MemoryPointer.from_string()
  • Regular string value (does not work yet)

Reference

Tested with

Ruby code

require 'ffi'

class Foo
  extend FFI::Library
  ffi_lib File.expand_path('fun.o')

  callback :incoming_rpc, [:string], :string
  attach_function :do_some_work, [:incoming_rpc, :string], :string

  def initialize
    output = do_some_work("Ruby init...") do |name|
      FFI::MemoryPointer.from_string("hello #{name}")
    end
    puts "Output: #{output.inspect}"
  end
end

Foo.new

C code

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef const char *callbkfn(char *);

extern char* do_some_work(callbkfn fn, char *name);

char* do_some_work(callbkfn fn, char* userdata) {
  printf("do_some_work param: %s\n", userdata);
  printf("callback output: %s\n", fn(strdup("Hello from C")));
  return strdup("Returned from C");
}

ext/ffi_c/Function.c Outdated Show resolved Hide resolved
ext/ffi_c/Function.c Outdated Show resolved Hide resolved
@datibbaw datibbaw force-pushed the tjerk/test-callback-string branch 2 times, most recently from 0e0c05b to 5da6c7b Compare March 16, 2020 23:54
@larskanis
Copy link
Member

Thank you for your contribution! I'm not the original author of the related code, but I'm pretty sure there is a reason why :string is not handled as a callback return value: It is not safe in most cases.

That's because the ruby String object that is returned by the block could get GC'ed as soon as the block has ended and so the string data can get invalid at this point in time. In fact the same is true for a FFI::MemoryPointer.from_string(s) but in that way you're getting aware about the fact, that you're returning a pointer. So you have to make sure, that the data the pointer is pointing to, is not GC'ed until it is processed by the C code. This can be solved by storing it in a variable outside of the block or in a instance variable or similar.

JRuby doesn't accept :string as a callback return value and raises a

TypeError: invalid callback return type: STRING

I think this check should be implemented here in C-FFI as well. I'm not convinced, that adding :string conversion to the callback return path is really a good idea, since it can lead to difficult to find GC related errors.

larskanis added a commit to larskanis/ffi that referenced this pull request Apr 16, 2020
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 follows the semantics for passing :string parameters to functions as implemented in Call.c.
That means in particular usage of StringValueCStr(), so that no zero bytes are allowed in the string.

Special care has to be taken regarding lifetime of the returned text pointer.
The C pointer to the string is only valid until the next GC triggering function is called, unless the String object is hold in Ruby outside of the callback block.
Even when the object is stored outside of the Ruby block, the text pointer can move when GC.compact is called.
So :string returning callbacks can safely be used only, when the referenced text is processed immediately by the calling C function.

Resolves ffi#751
larskanis added a commit to larskanis/ffi that referenced this pull request Apr 16, 2020
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 follows the semantics for passing :string parameters to functions as implemented in Call.c.
That means in particular usage of StringValueCStr(), so that no zero bytes are allowed in the string.

Special care has to be taken regarding lifetime of the returned text pointer.
The C pointer to the string is only valid until the next GC triggering function is called, unless the String object is hold in Ruby outside of the callback block.
Even when the object is stored outside of the Ruby block, the text pointer can move when GC.compact is called.
So :string returning callbacks can safely be used only, when the referenced text is processed immediately by the calling C function.

Resolves ffi#751
larskanis added a commit to larskanis/ffi that referenced this pull request May 23, 2020
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 ffi#751
larskanis added a commit to larskanis/ffi that referenced this pull request May 24, 2020
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
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