Query ruby thread and GVL states instead of relying on our call frame…

… for callbacks This fixes #527 and other interoperability issues that can happen, when a callback is called without a valid call frame. The change is enabled for ruby-2.3+ only, since it requires the functions ruby_native_thread_p() and ruby_thread_has_gvl_p(). The creation of a call frame is not (yet) removed, because it still carries callback expections around.
ffi · Jul 26, 2017 · 743fae1 · 743fae1
1 parent a3e6f83
commit 743fae1
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 30 deletions.
diff --git a/ext/ffi_c/Call.c b/ext/ffi_c/Call.c
@@ -113,7 +113,7 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
         Type* paramType = paramTypes[i];
         int type;
 
-        
+
         if (unlikely(paramType->nativeType == NATIVE_MAPPED)) {
             VALUE values[] = { argv[argidx], Qnil };
             argv[argidx] = rb_funcall2(((MappedType *) paramType)->rbConverter, id_to_native, 2, values);
@@ -294,8 +294,8 @@ rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,
 
             case NATIVE_STRING:
                 if (type == T_NIL) {
-                    param->ptr = NULL; 
-                
+                    param->ptr = NULL;
+
                 } else {
                     if (rb_safe_level() >= 1 && OBJ_TAINTED(argv[argidx])) {
                         rb_raise(rb_eSecurityError, "Unsafe string parameter");
@@ -342,9 +342,13 @@ static VALUE
 call_blocking_function(void* data)
 {
     rbffi_blocking_call_t* b = (rbffi_blocking_call_t *) data;
+#ifndef HAVE_RUBY_THREAD_HAS_GVL_P
     b->frame->has_gvl = false;
+#endif
     ffi_call(&b->cif, FFI_FN(b->function), b->retval, b->ffiValues);
+#ifndef HAVE_RUBY_THREAD_HAS_GVL_P
     b->frame->has_gvl = true;
+#endif
 
     return Qnil;
 }
@@ -373,9 +377,9 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
     FFIStorage* params;
     VALUE rbReturnValue;
     rbffi_frame_t frame = { 0 };
-    
+
     retval = alloca(MAX(fnInfo->ffi_cif.rtype->size, FFI_SIZEOF_ARG));
-    
+
     if (unlikely(fnInfo->blocking)) {
         rbffi_blocking_call_t* bc;
 
@@ -405,7 +409,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
             fnInfo->parameterCount, fnInfo->parameterTypes, params, ffiValues,
             fnInfo->callbackParameters, fnInfo->callbackCount, fnInfo->rbEnums);
 
-        rbffi_frame_push(&frame); 
+        rbffi_frame_push(&frame);
         rb_rescue2(rbffi_do_blocking_call, (VALUE) bc, rbffi_save_frame_exception, (VALUE) &frame, rb_eException, (VALUE) 0);
         rbffi_frame_pop(&frame);
 
@@ -416,7 +420,7 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
         xfree(bc->retval);
         xfree(bc);
 #endif
-    
+
     } else {
 
         ffiValues = ALLOCA_N(void *, fnInfo->parameterCount);
@@ -433,15 +437,15 @@ rbffi_CallFunction(int argc, VALUE* argv, void* function, FunctionType* fnInfo)
 
     if (unlikely(!fnInfo->ignoreErrno)) {
         rbffi_save_errno();
-    }    
+    }
 
     if (RTEST(frame.exc) && frame.exc != Qnil) {
         rb_exc_raise(frame.exc);
     }
 
     RB_GC_GUARD(rbReturnValue) = rbffi_NativeValue_ToRuby(fnInfo->returnType, fnInfo->rbReturnType, retval);
     RB_GC_GUARD(fnInfo->rbReturnType);
-    
+
     return rbReturnValue;
 }
 
@@ -458,7 +462,7 @@ getPointer(VALUE value, int type)
         return memory != NULL ? memory->address : NULL;
 
     } else if (type == T_STRING) {
-        
+
         return StringValuePtr(value);
 
     } else if (type == T_NIL) {

diff --git a/ext/ffi_c/Call.h b/ext/ffi_c/Call.h
@@ -48,7 +48,7 @@ extern "C" {
 #if (defined(__i386__) || defined(__x86_64__)) && !(defined(_WIN32) || defined(__WIN32__))
 #  define BYPASS_FFI 1
 #endif
-    
+
 typedef union {
 #ifdef USE_RAW
     signed int s8, s16, s32;
@@ -70,7 +70,7 @@ typedef union {
     double f64;
     long double ld;
 } FFIStorage;
- 
+
 extern void rbffi_Call_Init(VALUE moduleFFI);
 
 extern void rbffi_SetupCallParams(int argc, VALUE* argv, int paramCount, Type** paramTypes,

diff --git a/ext/ffi_c/Function.c b/ext/ffi_c/Function.c
@@ -102,6 +102,27 @@ static VALUE async_cb_call(void *);
 extern void *rb_thread_call_with_gvl(void *(*func)(void *), void *data1);
 #endif
 
+#ifdef HAVE_RUBY_THREAD_HAS_GVL_P
+extern int ruby_thread_has_gvl_p(void);
+#define rbffi_thread_has_gvl_p(frame) ruby_thread_has_gvl_p()
+#else
+static int rbffi_thread_has_gvl_p(rbffi_frame_t *frame)
+{
+    return frame != NULL && frame->has_gvl;
+}
+#endif
+
+#ifdef HAVE_RUBY_NATIVE_THREAD_P
+extern int ruby_native_thread_p(void);
+#define rbffi_native_thread_p(frame) ruby_native_thread_p()
+#else
+static int rbffi_native_thread_p(rbffi_frame_t *frame)
+{
+    return frame != NULL;
+}
+#endif
+
+
 VALUE rbffi_FunctionClass = Qnil;
 
 #if defined(DEFER_ASYNC_CALLBACK)
@@ -471,13 +492,13 @@ callback_invoke(ffi_cif* cif, void* retval, void** parameters, void* user_data)
     cb.frame = rbffi_frame_current();
 
     if (cb.frame != NULL) cb.frame->exc = Qnil;
-    if (cb.frame != NULL && cb.frame->has_gvl) {
-        callback_with_gvl(&cb);
 
-#if defined(HAVE_RB_THREAD_CALL_WITH_GVL)
-    } else if (cb.frame != NULL) {
+    if (rbffi_native_thread_p(cb.frame)) {
+      if(rbffi_thread_has_gvl_p(cb.frame)) {
+        callback_with_gvl(&cb);
+      } else {
         rb_thread_call_with_gvl(callback_with_gvl, &cb);
-#endif
+      }
 #if defined(DEFER_ASYNC_CALLBACK) && !defined(_WIN32)
     } else {
         bool empty = false;

diff --git a/ext/ffi_c/Thread.c b/ext/ffi_c/Thread.c
@@ -69,13 +69,15 @@ rbffi_frame_current(void)
 #endif
 }
 
-void 
+void
 rbffi_frame_push(rbffi_frame_t* frame)
 {
     memset(frame, 0, sizeof(*frame));
+#ifndef HAVE_RUBY_THREAD_HAS_GVL_P
     frame->has_gvl = true;
+#endif
     frame->exc = Qnil;
-    
+
 #ifdef _WIN32
     frame->prev = TlsGetValue(frame_thread_key);
     TlsSetValue(frame_thread_key, frame);
@@ -86,7 +88,7 @@ rbffi_frame_push(rbffi_frame_t* frame)
 #endif
 }
 
-void 
+void
 rbffi_frame_pop(rbffi_frame_t* frame)
 {
 #ifdef _WIN32
@@ -117,13 +119,13 @@ rbffi_blocking_thread(void* args)
     struct BlockingThread* thr = (struct BlockingThread *) args;
     char c = 1;
     VALUE retval;
-    
+
     retval = (*thr->fn)(thr->data);
-    
+
     pthread_testcancel();
 
     thr->retval = retval;
-    
+
     write(thr->wrfd, &c, sizeof(c));
 
     return NULL;
@@ -134,7 +136,7 @@ wait_for_thread(void *data)
 {
     struct BlockingThread* thr = (struct BlockingThread *) data;
     char c;
-    
+
     if (read(thr->rdfd, &c, 1) < 1) {
         rb_thread_wait_fd(thr->rdfd);
         while (read(thr->rdfd, &c, 1) < 1 && rb_io_wait_readable(thr->rdfd) == Qtrue) {
@@ -165,7 +167,7 @@ rbffi_thread_blocking_region(VALUE (*func)(void *), void *data1, void (*ubf)(voi
     struct BlockingThread* thr;
     int fd[2];
     VALUE exc;
-    
+
     if (pipe(fd) < 0) {
         rb_raise(rb_eSystemCallError, "pipe(2) failed");
         return Qnil;
@@ -191,7 +193,7 @@ rbffi_thread_blocking_region(VALUE (*func)(void *), void *data1, void (*ubf)(voi
 
     exc = rb_rescue2(wait_for_thread, (VALUE) thr, cleanup_blocking_thread, (VALUE) thr,
         rb_eException);
-    
+
     pthread_join(thr->tid, NULL);
     close(fd[1]);
     close(fd[0]);
@@ -347,6 +349,6 @@ rbffi_Thread_Init(VALUE moduleFFI)
 #ifdef _WIN32
     frame_thread_key = TlsAlloc();
 #else
-    pthread_key_create(&thread_data_key, thread_data_free);    
+    pthread_key_create(&thread_data_key, thread_data_free);
 #endif
 }
diff --git a/ext/ffi_c/Thread.h b/ext/ffi_c/Thread.h
@@ -66,7 +66,9 @@ typedef struct rbffi_frame {
     struct thread_data* td;
 #endif
     struct rbffi_frame* prev;
+#ifndef HAVE_RUBY_THREAD_HAS_GVL_P
     bool has_gvl;
+#endif
     VALUE exc;
 } rbffi_frame_t;
 

diff --git a/ext/ffi_c/extconf.rb b/ext/ffi_c/extconf.rb
@@ -11,7 +11,7 @@
   $CFLAGS.gsub!(/[\s+]-std=[^\s]+/, '')
   # solaris 10 needs -c99 for <stdbool.h>
   $CFLAGS << " -std=c99" if RbConfig::CONFIG['host_os'] =~ /solaris(!?2\.11)/
-  
+
   if ENV['RUBY_CC_VERSION'].nil? && (pkg_config("libffi") ||
      have_header("ffi.h") ||
      find_header("ffi.h", "/usr/local/include", "/usr/include/ffi"))
@@ -29,6 +29,11 @@
   have_func('rb_thread_blocking_region')
   have_func('rb_thread_call_with_gvl')
   have_func('rb_thread_call_without_gvl')
+  have_func('ruby_native_thread_p')
+  if RbConfig::CONFIG['host_os'].downcase !~ /darwin/ || RUBY_VERSION >= "2.3.0"
+    # On OSX ruby_thread_has_gvl_p is detected but fails at runtime for ruby < 2.3.0
+    have_func('ruby_thread_has_gvl_p')
+  end
 
   if libffi_ok
     have_func('ffi_prep_cif_var')
@@ -42,7 +47,7 @@
   $defs << "-DFFI_BUILDING" if RbConfig::CONFIG['host_os'] =~ /mswin/ # for compatibility with newer libffi
 
   create_header
-  
+
   $LOCAL_LIBS << " ./libffi/.libs/libffi_convenience.lib" if !libffi_ok && RbConfig::CONFIG['host_os'] =~ /mswin/
 
   create_makefile("ffi_c")
@@ -62,7 +67,7 @@
       end
     end
   end
-  
+
 else
   File.open("Makefile", "w") do |mf|
     mf.puts "# Dummy makefile for non-mri rubies"