Check attach result before using resulting JNIEnv pointer in callback…

… code

The success of attaching to the JVM was checked to late in callback.c.
After trying to attach the native thread to the JVM, the JNIEnv pointer
is passed to `get_thread_storage`, which uses it to access the JVM.

At this point it is assumed, that the JNIEnv pointer is valid and can
be used. This is not correct, as the attach can fail and if so the
JNIEnv pointer is not valid.

The pointer is later validated, but to late, so a SEGFAULT occurs.

The result check needs to be moved directly after the attachment
code, to be effective.

CHANGES.md

@@ -12,6 +12,7 @@ Bug Fixes
 ---------
 * [#925](https://github.com/java-native-access/jna/issues/925): Optimize `Structure#validate` and prevent `ArrayIndexOutOfBoundsException` in `SAFEARRAY#read` for zero dimensions - [@matthiasblaesing](https://github.com/matthiasblaesing).
 * [#958](https://github.com/java-native-access/jna/issues/958): Update for PR 863: Old toolchains produce binaries without hard-/softfloat markers. Rasbian is missinng the markers and the oracle JDK is also affected. For hardfloat detection now also the Arm EABI section is also considered - [@matthiasblaesing](https://github.com/matthiasblaesing).
+* [#974](https://github.com/java-native-access/jna/issues/974): If the callback code failed to attach to the JVM, this lead to a segfault. The success of attaching to the JVM was checked to late and an invalid `JNIEnv` pointer was used to access the JVM - [@matthiasblaesing](https://github.com/matthiasblaesing).
 
 Release 4.5.1
 =============

build.xml

@@ -69,7 +69,7 @@
   <!-- jnidispatch library release version -->
   <property name="jni.major" value="5"/>
   <property name="jni.minor" value="2"/>
-  <property name="jni.revision" value="0"/>
+  <property name="jni.revision" value="2"/>
   <property name="jni.build" value="0"/> <!--${build.number}-->
   <property name="jni.version" value="${jni.major}.${jni.minor}.${jni.revision}"/>
   <property name="jni.md5" value="74e8f8e397c43487738c5c1f1363498b"/>

native/callback.c

@@ -686,6 +686,14 @@ dispatch_callback(ffi_cif* cif, void* resp, void** cbargs, void* user_data) {
     else {
       attach_status = (*jvm)->AttachCurrentThread(jvm, (void *)&env, &args);
     }
+    if (attach_status != JNI_OK) {
+      free((void *)args.name);
+      if (args.group) {
+        (*env)->DeleteWeakGlobalRef(env, args.group);
+      }
+      fprintf(stderr, "JNA: Can't attach native thread to VM for callback: %d (check stacksize for callbacks)\n", attach_status);
+      return;
+    }
     tls = get_thread_storage(env);
     if (tls) {
       snprintf(tls->name, sizeof(tls->name), "%s", args.name ? args.name : "<unconfigured native thread>");
@@ -694,15 +702,11 @@ dispatch_callback(ffi_cif* cif, void* resp, void** cbargs, void* user_data) {
     }
     // Dispose of allocated memory
     free((void *)args.name);
-    if (attach_status != JNI_OK) {
-      fprintf(stderr, "JNA: Can't attach native thread to VM for callback: %d\n", attach_status);
-      return;
-    }
     if (args.group) {
       (*env)->DeleteWeakGlobalRef(env, args.group);
     }
   }
-						
+
   if (!tls) {
     fprintf(stderr, "JNA: couldn't obtain thread-local storage\n");
     return;

native/testlib.c

@@ -54,7 +54,7 @@ typedef __int64 int64_t;
 #define EXPORT __declspec(dllexport)
 #define SLEEP(MS) Sleep(MS)
 #define THREAD_T DWORD
-#define THREAD_CREATE(TP, FN, DATA) CreateThread(NULL, 0, FN, DATA, 0, TP)
+#define THREAD_CREATE(TP, FN, DATA, STACKSIZE) CreateThread(NULL, STACKSIZE, FN, DATA, 0, TP)
 #define THREAD_EXIT() ExitThread(0)
 #define THREAD_FUNC(FN,ARG) DWORD WINAPI FN(LPVOID ARG)
 #define THREAD_CURRENT() GetCurrentThreadId()
@@ -69,7 +69,15 @@ typedef __int64 int64_t;
 #include <pthread.h>
 #define SLEEP(MS) usleep(MS*1000)
 #define THREAD_T pthread_t
-#define THREAD_CREATE(TP, FN, DATA) pthread_create(TP, NULL, FN, DATA)
+#define THREAD_CREATE(TP, FN, DATA, STACKSIZE) {\
+  pthread_attr_t attr;\
+  pthread_attr_init(&attr);\
+  if (STACKSIZE > 0) {\
+    pthread_attr_setstacksize(&attr, STACKSIZE);\
+  }\
+  pthread_create(TP, &attr, FN, DATA);\
+  pthread_attr_destroy(&attr);\
+}
 #define THREAD_EXIT() pthread_exit(NULL)
 #define THREAD_FUNC(FN,ARG) void* FN(void *ARG)
 #define THREAD_RETURN return NULL
@@ -676,15 +684,15 @@ static THREAD_FUNC(thread_function, arg) {
 }
 
 EXPORT void
-callVoidCallbackThreaded(void (*func)(void), int n, int ms, const char* name) {
+callVoidCallbackThreaded(void (*func)(void), int n, int ms, const char* name, int stacksize) {
   THREAD_T thread;
   thread_data* data = (thread_data*)malloc(sizeof(thread_data));
 
   data->repeat_count = n;
   data->sleep_time = ms;
   data->func = func;
   snprintf(data->name, sizeof(data->name), "%s", name);
-  THREAD_CREATE(&thread, &thread_function, data);
+  THREAD_CREATE(&thread, &thread_function, data, stacksize);
 }
 
 EXPORT int 

test/com/sun/jna/CallbacksTest.java

@@ -127,7 +127,7 @@ interface VoidCallback extends Callback {
             void callback();
         }
         void callVoidCallback(VoidCallback c);
-        void callVoidCallbackThreaded(VoidCallback c, int count, int ms, String name);
+        void callVoidCallbackThreaded(VoidCallback c, int count, int ms, String name, int stacksize);
         interface VoidCallbackCustom extends Callback {
             void customMethodName();
         }
@@ -1215,7 +1215,7 @@ protected void callThreadedCallback(TestLibrary.VoidCallback cb,
         if (cti != null) {
             Native.setCallbackThreadInitializer(cb, cti);
         }
-        lib.callVoidCallbackThreaded(cb, repeat, sleepms, getName());
+        lib.callVoidCallbackThreaded(cb, repeat, sleepms, getName(), 0);
 
         long start = System.currentTimeMillis();
         while (called[0] < returnAfter) {
@@ -1304,6 +1304,25 @@ public void callback() {
         waitFor(t[0]);
     }
 
+    public void testSmallStackCallback() throws Exception {
+        // This test runs the callback in a thread, that is allocated a very
+        // small size. It was observed on linux amd64, that a library allocated
+        // a stack size of 64kB, this prevented the JVM to attach to that
+        // thread. The JNIEnv pointer was not checked and this lead to a
+        // hard crash of the JVM.
+        TestLibrary.VoidCallback cb = new TestLibrary.VoidCallback() {
+            @Override
+            public void callback() {
+                System.out.println("Callback called");
+            }
+        };
+
+        lib.callVoidCallbackThreaded(cb, 1, 0, "Test Callback", 32 * 1024);
+
+        // Give the JVM enough time to run the call back
+        Thread.sleep(1 * 1000);
+    }
+
     // Detach preference is indicated by the initializer.  Thread is attached
     // as daemon to avoid VM having to wait for it.
     public void testCallbackThreadPersistence() throws Exception {

test/com/sun/jna/DirectCallbacksTest.java

@@ -73,7 +73,7 @@ public static class DirectTestLibrary implements TestLibrary {
         @Override
         public native void callVoidCallback(VoidCallback c);
         @Override
-        public native void callVoidCallbackThreaded(VoidCallback c, int count, int ms, String name);
+        public native void callVoidCallbackThreaded(VoidCallback c, int count, int ms, String name, int stacksize);
 
         @Override
         public native int callInt32Callback(CustomCallback cb, int arg1, int arg2);