From e2b1c7e301bd4d74e622c95f0d5c108edb7b29fc Mon Sep 17 00:00:00 2001
From: Lars Kanis <lars@greiz-reinsdorf.de>
Date: Sat, 23 May 2020 23:27:10 +0200
Subject: [PATCH] Reject callback with :string return type

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 #751
---
 ext/ffi_c/FunctionInfo.c         | 3 +--
 lib/ffi/library.rb               | 6 +++++-
 lib/ffi/struct.rb                | 6 +++++-
 spec/ffi/callback_spec.rb        | 9 +++++++++
 spec/ffi/struct_callback_spec.rb | 8 ++++++++
 5 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/ext/ffi_c/FunctionInfo.c b/ext/ffi_c/FunctionInfo.c
index 8085c870e..b7b23a75f 100644
--- a/ext/ffi_c/FunctionInfo.c
+++ b/ext/ffi_c/FunctionInfo.c
@@ -172,7 +172,7 @@ fntype_initialize(int argc, VALUE* argv, VALUE self)
         VALUE typeName = rb_funcall2(rbReturnType, rb_intern("inspect"), 0, NULL);
         rb_raise(rb_eTypeError, "Invalid return type (%s)", RSTRING_PTR(typeName));
     }
-    
+
     if (rb_obj_is_kind_of(fnInfo->rbReturnType, rbffi_StructByValueClass)) {
         fnInfo->hasStruct = true;
     }
@@ -180,7 +180,6 @@ fntype_initialize(int argc, VALUE* argv, VALUE self)
     Data_Get_Struct(fnInfo->rbReturnType, Type, fnInfo->returnType);
     fnInfo->ffiReturnType = fnInfo->returnType->ffiType;
 
-
 #if defined(X86_WIN32)
     rbConventionStr = (rbConvention != Qnil) ? rb_funcall2(rbConvention, rb_intern("to_s"), 0, NULL) : Qnil;
     fnInfo->abi = (rbConventionStr != Qnil && strcmp(StringValueCStr(rbConventionStr), "stdcall") == 0)
diff --git a/lib/ffi/library.rb b/lib/ffi/library.rb
index 8d1effa68..602d90604 100644
--- a/lib/ffi/library.rb
+++ b/lib/ffi/library.rb
@@ -394,7 +394,11 @@ def callback(*args)
       options = Hash.new
       options[:convention] = ffi_convention
       options[:enums] = @ffi_enums if defined?(@ffi_enums)
-      cb = FFI::CallbackInfo.new(find_type(ret), native_params, options)
+      ret_type = find_type(ret)
+      if ret_type == FFI::Type::Builtin::STRING
+        raise TypeError, ":string is not allowed as return type of callbacks"
+      end
+      cb = FFI::CallbackInfo.new(ret_type, native_params, options)
 
       # Add to the symbol -> type map (unless there was no name)
       unless name.nil?
diff --git a/lib/ffi/struct.rb b/lib/ffi/struct.rb
index e548d8f48..86310ed7b 100644
--- a/lib/ffi/struct.rb
+++ b/lib/ffi/struct.rb
@@ -228,7 +228,11 @@ def layout(*spec)
 
       def callback(params, ret)
         mod = enclosing_module
-        FFI::CallbackInfo.new(find_type(ret, mod), params.map { |e| find_type(e, mod) })
+        ret_type = find_type(ret, mod)
+        if ret_type == FFI::Type::Builtin::STRING
+          raise TypeError, ":string is not allowed as return type of callbacks"
+        end
+        FFI::CallbackInfo.new(ret_type, params.map { |e| find_type(e, mod) })
       end
 
       def packed(packed = 1)
diff --git a/spec/ffi/callback_spec.rb b/spec/ffi/callback_spec.rb
index be79ad2ff..019fd293e 100644
--- a/spec/ffi/callback_spec.rb
+++ b/spec/ffi/callback_spec.rb
@@ -309,6 +309,15 @@ class S8F32S32 < FFI::Struct
       expect(s2[:f32]).to be_within(0.0000001).of 1.234567
     end
 
+    it "returning :string is rejected as typedef" do
+      expect {
+        Module.new do
+          extend FFI::Library
+          ffi_lib TestLibrary::PATH
+          callback :cbVrA, [], :string
+        end
+      }.to raise_error(TypeError)
+    end
 
     it "global variable" do
       proc = Proc.new { 0x1e }
diff --git a/spec/ffi/struct_callback_spec.rb b/spec/ffi/struct_callback_spec.rb
index 7cab301bb..be01e8557 100644
--- a/spec/ffi/struct_callback_spec.rb
+++ b/spec/ffi/struct_callback_spec.rb
@@ -66,4 +66,12 @@ class TestStruct < FFI::Struct
     expect(fn.respond_to?(:call)).to be true
     expect(fn.call(1, 2)).to eq(3)
   end
+
+  it "callback returning :string is rejected in struct" do
+    expect {
+      Class.new(FFI::Struct) do
+        layout :function1, callback([:int, :int], :string)
+      end
+    }.to raise_error(TypeError)
+  end
 end