8238676: jni crashes on accessing it from process exit hook

Reviewed-by: fparain, gziemski

make/test/JtregNativeHotspot.gmk

@@ -881,7 +881,7 @@ ifeq ($(call isTargetOs, windows), true)
     BUILD_HOTSPOT_JTREG_EXECUTABLES_CFLAGS_exeFPRegs := -MT
     BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c
     BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exejvm-test-launcher := jvm.lib
-
+    BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit := jvm.lib
 else
     BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exejvm-test-launcher := -ljvm
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libbootclssearch_agent += -lpthread
@@ -1517,6 +1517,7 @@ else
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libgetphase001 += -lpthread
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libgetphase002 += -lpthread
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libterminatedThread += -lpthread
+    BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit += -ljvm
 endif
 
 # This evaluation is expensive and should only be done if this target was

src/hotspot/share/prims/jni.cpp

@@ -3681,7 +3681,8 @@ extern const struct JNIInvokeInterface_ jni_InvokeInterface;
 
 // Global invocation API vars
 volatile int vm_created = 0;
-// Indicate whether it is safe to recreate VM
+// Indicate whether it is safe to recreate VM. Recreation is only
+// possible after a failed initial creation attempt in some cases.
 volatile int safe_to_recreate_vm = 1;
 struct JavaVM_ main_vm = {&jni_InvokeInterface};
 
@@ -3751,8 +3752,14 @@ static jint JNI_CreateJavaVM_inner(JavaVM **vm, void **penv, void *args) {
   if (Atomic::xchg(&vm_created, 1) == 1) {
     return JNI_EEXIST;   // already created, or create attempt in progress
   }
+
+  // If a previous creation attempt failed but can be retried safely,
+  // then safe_to_recreate_vm will have been reset to 1 after being
+  // cleared here. If a previous creation attempt succeeded and we then
+  // destroyed that VM, we will be prevented from trying to recreate
+  // the VM in the same process, as the value will still be 0.
   if (Atomic::xchg(&safe_to_recreate_vm, 0) == 0) {
-    return JNI_ERR;  // someone tried and failed and retry not allowed.
+    return JNI_ERR;
   }
 
   assert(vm_created == 1, "vm_created is true during the creation");
@@ -3945,9 +3952,14 @@ static jint attach_current_thread(JavaVM *vm, void **penv, void *_args, bool dae
 
   Thread* t = Thread::current_or_null();
   if (t != NULL) {
-    // If the thread has been attached this operation is a no-op
-    *(JNIEnv**)penv = ((JavaThread*) t)->jni_environment();
-    return JNI_OK;
+    // If executing from an atexit hook we may be in the VMThread.
+    if (t->is_Java_thread()) {
+      // If the thread has been attached this operation is a no-op
+      *(JNIEnv**)penv = ((JavaThread*) t)->jni_environment();
+      return JNI_OK;
+    } else {
+      return JNI_ERR;
+    }
   }
 
   // Create a thread and mark it as attaching so it will be skipped by the
@@ -4045,7 +4057,7 @@ static jint attach_current_thread(JavaVM *vm, void **penv, void *_args, bool dae
 jint JNICALL jni_AttachCurrentThread(JavaVM *vm, void **penv, void *_args) {
   HOTSPOT_JNI_ATTACHCURRENTTHREAD_ENTRY(vm, penv, _args);
   if (vm_created == 0) {
-  HOTSPOT_JNI_ATTACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
+    HOTSPOT_JNI_ATTACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
     return JNI_ERR;
   }
 
@@ -4058,18 +4070,30 @@ jint JNICALL jni_AttachCurrentThread(JavaVM *vm, void **penv, void *_args) {
 
 jint JNICALL jni_DetachCurrentThread(JavaVM *vm)  {
   HOTSPOT_JNI_DETACHCURRENTTHREAD_ENTRY(vm);
+  if (vm_created == 0) {
+    HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_ERR);
+    return JNI_ERR;
+  }
 
   JNIWrapper("DetachCurrentThread");
 
+  Thread* current = Thread::current_or_null();
+
   // If the thread has already been detached the operation is a no-op
-  if (Thread::current_or_null() == NULL) {
+  if (current == NULL) {
     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_OK);
     return JNI_OK;
   }
 
+  // If executing from an atexit hook we may be in the VMThread.
+  if (!current->is_Java_thread()) {
+    HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
+    return JNI_ERR;
+  }
+
   VM_Exit::block_if_vm_exited();
 
-  JavaThread* thread = JavaThread::current();
+  JavaThread* thread = (JavaThread*) current;
   if (thread->has_last_Java_frame()) {
     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
     // Can't detach a thread that's running java, that can't work.

test/hotspot/jtreg/runtime/jni/atExit/TestAtExit.java

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+/*
+ * @test
+ * @bug 8238676
+ * @summary Check that attempting to use the JNI invocation API from an
+ *          atexit handler fails as expected without crashing.
+ *
+ * @library /test/lib
+ * @run main/othervm/native TestAtExit
+ */
+
+public class TestAtExit {
+
+    // Using a nested class that invokes an enclosing method makes it
+    // easier to setup and use the native library.
+    static class Tester {
+        static {
+            System.loadLibrary("atExit");
+        }
+
+        // Record the fact we are using System.exit for termination
+        static native void setUsingSystemExit();
+
+        public static void main(String[] args) throws Exception {
+            if (args.length > 0) {
+                setUsingSystemExit();
+                System.exit(0);
+            }
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        // We mustn't load Tester in this VM so we exec by name.
+        String main = "TestAtExit$Tester";
+
+        String jlp = "-Djava.library.path=" + System.getProperty("test.nativepath");
+        // First run will terminate via DestroyJavaVM
+        OutputAnalyzer output = ProcessTools.executeTestJvm(jlp, main);
+        output.shouldNotContain("Unexpected");
+        output.shouldHaveExitValue(0);
+        output.reportDiagnosticSummary();
+
+        // Second run will terminate via System.exit()
+        output = ProcessTools.executeTestJvm(jlp, main, "doExit");
+        output.shouldNotContain("Unexpected");
+        output.shouldHaveExitValue(0);
+        output.reportDiagnosticSummary();
+    }
+}

test/hotspot/jtreg/runtime/jni/atExit/libatExit.c

@@ -0,0 +1,113 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "jni.h"
+
+static JavaVM *jvm;
+
+static const char* jni_error_code(int ret) {
+  switch(ret) {
+  case JNI_OK: return "JNI_OK";
+  case JNI_ERR: return "JNI_ERR";
+  case JNI_EDETACHED: return "JNI_EDETACHED";
+  case JNI_EVERSION: return "JNI_EVERSION";
+  case JNI_ENOMEM: return "JNI_ENOMEM";
+  case JNI_EEXIST: return "JNI_EEXIST";
+  case JNI_EINVAL: return "JNI_EINVAL";
+  default: return "Invalid JNI error code";
+  }
+}
+
+static void report(const char* func, int ret_actual, int ret_expected) {
+  const char* ret = jni_error_code(ret_actual);
+  if (ret_actual == ret_expected) {
+    printf("%s returned %s as expected\n", func, ret);
+  } else {
+    printf("Unexpected JNI return code %s from %s\n", ret, func);
+  }
+}
+
+static int using_system_exit = 0; // Not System.exit by default
+
+JNIEXPORT
+void JNICALL Java_TestAtExit_00024Tester_setUsingSystemExit(JNIEnv* env, jclass c) {
+  using_system_exit = 1;
+}
+
+void at_exit_handler(void) {
+  printf("In at_exit_handler\n");
+
+  // We've saved the JavaVM from OnLoad time so we first try to
+  // get a JNIEnv for the current thread.
+  JNIEnv *env;
+  jint res = (*jvm)->GetEnv(jvm, (void **)&env, JNI_VERSION_1_2);
+  report("GetEnv", res, JNI_EDETACHED);
+  if (res == JNI_EDETACHED) {
+
+    // Test all of the Invocation API functions
+
+    res = (*jvm)->AttachCurrentThreadAsDaemon(jvm, (void **)&env, NULL);
+    report("AttachCurrentThreadAsDaemon", res, JNI_ERR);
+    res = (*jvm)->AttachCurrentThread(jvm, (void **)&env, NULL);
+    report("AttachCurrentThread", res, JNI_ERR);
+    res = (*jvm)->DetachCurrentThread(jvm);
+    report("DetachCurrentThread", res, JNI_ERR);
+
+    JavaVMInitArgs args;
+    args.version = JNI_VERSION_1_2;
+    res = JNI_GetDefaultJavaVMInitArgs(&args);
+    report("JNI_GetDefaultJavaVMInitArgs", res, JNI_OK);
+
+    JavaVM* jvm_p[1];
+    int nVMs;
+    res = JNI_GetCreatedJavaVMs(jvm_p, 1, &nVMs);
+    report("JNI_GetCreatedJavaVMs", res, JNI_OK);
+    // Whether nVMs is 0 or 1 depends on the termination path
+    if (nVMs == 0 && !using_system_exit) {
+      printf("Found 0 created VMs as expected\n");
+    } else if (nVMs == 1 && using_system_exit) {
+      printf("Found 1 created VM as expected\n");
+    } else {
+      printf("Unexpected number of created VMs: %d\n", nVMs);
+    }
+
+    res = (*jvm)->DestroyJavaVM(jvm);
+    report("DestroyJavaVM", res, JNI_ERR);
+
+    // Failure mode depends on the termination path
+    res = JNI_CreateJavaVM(jvm_p, (void**)&env, &args);
+    report("JNI_CreateJavaVM", res, using_system_exit ? JNI_EEXIST : JNI_ERR);
+  }
+  // else test has already failed
+}
+
+jint JNI_OnLoad(JavaVM *vm, void *reserved) {
+  printf("JNI_OnLoad: registering atexit handler\n");
+  jvm = vm;
+  atexit(at_exit_handler);
+
+  return JNI_VERSION_1_1;
+}