[GR-66212] Use the wrapper at the right place where the Java->native call occurs

PullRequest: truffleruby/4570

Author: eregon

.gitignore

@@ -48,7 +48,6 @@ src/main/c/*/Makefile
 !src/main/c/truffleposix/Makefile
 !src/main/c/yarp/Makefile
 src/main/c/cext-trampoline/trampoline.c
-src/main/c/cext/wrappers.c
 src/main/c/etc/constdefs.h
 src/main/c/spawn-helper/spawn-helper
 src/main/c/debug

CHANGELOG.md

@@ -9,6 +9,7 @@ Bug fixes:
 * Fix `Time.new` with String argument and handle nanoseconds correctly (#3836, @andrykonchin).
 * Fix a possible case of infinite recursion when implementing `frozen?` in a native extension (@nirvdrum).
 * Fix parameters forwarding to a method call executed with `Kernel#eval` (@andrykonchin).
+* Fix segfaults in native extensions when the reference processing thread is interrupted and would `longjmp()` incorrectly (#3903, @eregon).
 
 Compatibility:
 

lib/cext/ABI_check.txt

@@ -1 +1 @@
-1
+2

lib/truffle/truffle/cext.rb

@@ -38,7 +38,16 @@ module Truffle::CExt
   end
 
   SETUP_WRAPPERS = -> lib do
-    # signature starts with an extra pointer which is for passing the native function to call
+    # These wrappers must be used when calling native extension functions,
+    # so that setjmp() is done before calling the native extension function,
+    # and if the native extension function (potentially through other functions) calls a Ruby C API function
+    # and if that throws a Java exception, then on return from that Ruby C API function a longjmp()
+    # will be done to unwind the native stack.
+    # It is essential that only native frames and no Java frames are unwinded with longjmp(),
+    # which means these wrappers must be used exactly when calling into native functions, not before or after.
+    # Sulong-executed C code counts as not-native in this context.
+
+    # The signature starts with an extra pointer which is for passing the native function to call
     POINTER_TO_POINTER_WRAPPERS = [nil] + (1..16).map do |n|
       sig = -"(pointer,#{(['pointer'] * n).join(',')}):pointer"
       Primitive.interop_eval_nfi(sig).bind(lib[:"rb_tr_setjmp_wrapper_pointer#{n}_to_pointer"])
@@ -1740,16 +1749,15 @@ def RTYPEDDATA(object)
     if Truffle::Interop.null?(marker_function)
       nil
     else
-      -> { Primitive.interop_execute(POINTER_TO_VOID_WRAPPER, [marker_function, struct]) }
+      -> { Primitive.interop_execute(marker_function, [struct]) }
     end
   end
 
   private def data_sizer(sizer_function, rtypeddata)
-    raise unless sizer_function.respond_to?(:call)
     use_cext_lock = Primitive.use_cext_lock?
     proc {
       Primitive.call_with_cext_lock_and_frame(
-        POINTER_TO_SIZE_T_WRAPPER, [sizer_function, rtypeddata],
+        sizer_function, [rtypeddata],
         Primitive.caller_special_variables_if_available, nil, use_cext_lock)
     }
   end
@@ -1792,7 +1800,7 @@ def rb_data_typed_object_wrap(ruby_class, data, data_type, mark, free, size)
   end
 
   def run_data_finalizer(function, data, use_cext_lock)
-    Primitive.call_with_cext_lock_and_frame POINTER_TO_VOID_WRAPPER, [function, data], nil, nil, use_cext_lock
+    Primitive.call_with_cext_lock_and_frame function, [data], nil, nil, use_cext_lock
   end
 
   def run_marker(obj)

src/main/c/Makefile

@@ -43,7 +43,7 @@ all: $(TRAMPOLINE) $(LIBTRUFFLERUBY) openssl/openssl.$(DLEXT) zlib/zlib.$(DLEXT)
 clean: clean_cexts clean_trampoline
 
 clean_trampoline:
-	$(Q) rm -f $(TRAMPOLINE) cext-trampoline/trampoline.c cext/wrappers.c cext-trampoline/*.o
+	$(Q) rm -f $(TRAMPOLINE) cext-trampoline/trampoline.c cext-trampoline/*.o
 
 clean_cexts:
 	$(Q) rm -f cext/Makefile cext/*.o $(LIBTRUFFLERUBY)

src/main/c/cext/data.c

@@ -29,27 +29,30 @@ struct RTypedData* rb_tr_rtypeddata_create(const rb_data_type_t *data_type, void
   return result;
 }
 
+void rb_tr_setjmp_wrapper_pointer1_to_void(void (*func)(VALUE arg), VALUE arg);
+size_t rb_tr_setjmp_wrapper_pointer1_to_size_t(size_t (*func)(const void *arg), const void *arg);
+
 void rb_tr_rdata_run_marker(struct RData* rdata) {
   void* data = rdata->data;
   RUBY_DATA_FUNC dmark = rdata->dmark;
   if (data != NULL && dmark != NULL) {
-    dmark(data);
+    rb_tr_setjmp_wrapper_pointer1_to_void(dmark, data);
   }
 }
 
 void rb_tr_rtypeddata_run_marker(struct RTypedData* rtypeddata) {
   void* data = rtypeddata->data;
   RUBY_DATA_FUNC dmark = rtypeddata->type->function.dmark;
   if (data != NULL && dmark != NULL) {
-    dmark(data);
+    rb_tr_setjmp_wrapper_pointer1_to_void(dmark, data);
   }
 }
 
 size_t rb_tr_rtypeddata_run_memsizer(struct RTypedData* rtypeddata) {
   void* data = rtypeddata->data;
   size_t (*dsize)(const void *) = rtypeddata->type->function.dsize;
   if (data != NULL && dsize != NULL) {
-    return dsize(data);
+    return rb_tr_setjmp_wrapper_pointer1_to_size_t(dsize, data);
   } else {
     return 0;
   }
@@ -62,7 +65,7 @@ void rb_tr_rdata_run_finalizer(struct RData* rdata) {
     if (dfree == RUBY_DEFAULT_FREE) {
       ruby_xfree(data);
     } else if (dfree != NULL) {
-      dfree(data);
+      rb_tr_setjmp_wrapper_pointer1_to_void(dfree, data);
     }
   }
   // Also free the struct RData
@@ -76,7 +79,7 @@ void rb_tr_rtypeddata_run_finalizer(struct RTypedData* rtypeddata) {
     if (dfree == RUBY_DEFAULT_FREE) {
       ruby_xfree(data);
     } else if (dfree != NULL) {
-      dfree(data);
+      rb_tr_setjmp_wrapper_pointer1_to_void(dfree, data);
     }
   }  // Also free the struct RTypedData
   free(rtypeddata);

tool/generate-cext-trampoline.rb

@@ -48,7 +48,7 @@
 functions = []
 
 Dir["src/main/c/cext/*.c"].sort.each do |file|
-  next if %w[cext_constants.c wrappers.c ruby.c st.c strlcpy.c].include?(File.basename(file))
+  next if %w[cext_constants.c ruby.c st.c strlcpy.c].include?(File.basename(file))
 
   contents = File.read(file)
   found_functions = contents.scan(function_regexp)
@@ -128,37 +128,31 @@ def struct_by_value?(type)
 
 C
 
-  File.open("src/main/c/cext/wrappers.c", "w") do |sulong|
-    sulong.puts <<C
-#include <ruby.h>
-
-C
-
-    signatures = [
-      ['rb_tr_setjmp_wrapper_void_to_void', '(void):void'],
-      ['rb_tr_setjmp_wrapper_pointer1_to_void', '(VALUE arg):void'],
-      ['rb_tr_setjmp_wrapper_pointer2_to_void', '(VALUE tracepoint, void *data):void'],
-      ['rb_tr_setjmp_wrapper_pointer3_to_void', '(VALUE val, ID id, VALUE *data):void'],
-      ['rb_tr_setjmp_wrapper_pointer3_to_int', '(VALUE key, VALUE val, VALUE arg):int'],
-      ['rb_tr_setjmp_wrapper_pointer1_to_size_t', '(const void *arg):size_t'],
-      ['rb_tr_setjmp_wrapper_int_pointer2_to_pointer', '(int argc, VALUE *argv, VALUE obj):VALUE'],
-      ['rb_tr_setjmp_wrapper_pointer2_int_to_pointer', '(VALUE g, VALUE h, int r):VALUE'],
-      ['rb_tr_setjmp_wrapper_pointer2_int_pointer2_to_pointer', '(VALUE yielded_arg, VALUE callback_arg, int argc, const VALUE *argv, VALUE blockarg):VALUE'],
+  signatures = [
+    ['rb_tr_setjmp_wrapper_void_to_void', '(void):void'],
+    ['rb_tr_setjmp_wrapper_pointer1_to_void', '(VALUE arg):void'],
+    ['rb_tr_setjmp_wrapper_pointer2_to_void', '(VALUE tracepoint, void *data):void'],
+    ['rb_tr_setjmp_wrapper_pointer3_to_void', '(VALUE val, ID id, VALUE *data):void'],
+    ['rb_tr_setjmp_wrapper_pointer3_to_int', '(VALUE key, VALUE val, VALUE arg):int'],
+    ['rb_tr_setjmp_wrapper_pointer1_to_size_t', '(const void *arg):size_t'],
+    ['rb_tr_setjmp_wrapper_int_pointer2_to_pointer', '(int argc, VALUE *argv, VALUE obj):VALUE'],
+    ['rb_tr_setjmp_wrapper_pointer2_int_to_pointer', '(VALUE g, VALUE h, int r):VALUE'],
+    ['rb_tr_setjmp_wrapper_pointer2_int_pointer2_to_pointer', '(VALUE yielded_arg, VALUE callback_arg, int argc, const VALUE *argv, VALUE blockarg):VALUE'],
+  ]
+  (1..16).each do |arity|
+    signatures << [
+      "rb_tr_setjmp_wrapper_pointer#{arity}_to_pointer",
+      "(#{(1..arity).map { |i| "VALUE arg#{i}" }.join(', ')}):VALUE"
     ]
-    (1..16).each do |arity|
-      signatures << [
-        "rb_tr_setjmp_wrapper_pointer#{arity}_to_pointer",
-        "(#{(1..arity).map { |i| "VALUE arg#{i}" }.join(', ')}):VALUE"
-      ]
-    end
+  end
 
-    signatures.each do |function_name, signature|
-      argument_types, return_type = signature.split(':')
-      argument_types = argument_types.delete_prefix('(').delete_suffix(')')
-      original_argument_types = argument_types
-      argument_types = '' if argument_types == 'void'
-      void = return_type == 'void'
-      f.puts <<C
+  signatures.each do |function_name, signature|
+    argument_types, return_type = signature.split(':')
+    argument_types = argument_types.delete_prefix('(').delete_suffix(')')
+    original_argument_types = argument_types
+    argument_types = '' if argument_types == 'void'
+    void = return_type == 'void'
+    f.puts <<C
 #{return_type} #{function_name}(#{return_type} (*func)(#{original_argument_types})#{', ' unless argument_types.empty?}#{argument_types}) {
   #{"#{return_type} result;" unless void}
 
@@ -179,13 +173,6 @@ def struct_by_value?(type)
 }
 
 C
-      sulong.puts <<C
-#{return_type} #{function_name}(#{return_type} (*func)(#{original_argument_types})#{', ' unless argument_types.empty?}#{argument_types}) {
-  #{'return ' unless void}func(#{argument_types.split(', ').map { |param| param[/\w+$/] }.join(', ')});
-}
-
-C
-    end
   end
 
   f.puts "\n// Functions\n\n"