8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM

Reviewed-by: jsjolen, gziemski

make/test/JtregNativeHotspot.gmk

@@ -1,5 +1,5 @@
 #
-# Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2015, 2023, 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
@@ -874,7 +874,7 @@ BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exesigtest := -ljvm
 
 ifeq ($(call isTargetOs, windows), true)
   BUILD_HOTSPOT_JTREG_EXECUTABLES_CFLAGS_exeFPRegs := -MT
-  BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libTestPsig.c libnativeStack.c
+  BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libTestPsig.c libnativeStack.c exeGetCreatedJavaVMs.c
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit := jvm.lib
   BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exedaemonDestroy := jvm.lib
 else
@@ -1516,6 +1516,7 @@ else
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit += -ljvm
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libCompleteExit += -lpthread
   BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack += -lpthread
+  BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exeGetCreatedJavaVMs := -ljvm -lpthread
 endif
 
 ifeq ($(ASAN_ENABLED), true)

src/hotspot/share/prims/jni.cpp

@@ -3472,7 +3472,14 @@ struct JNINativeInterface_* jni_functions_nocheck() {
 extern const struct JNIInvokeInterface_ jni_InvokeInterface;
 
 // Global invocation API vars
-volatile int vm_created = 0;
+enum VM_Creation_State {
+  NOT_CREATED = 0,
+  IN_PROGRESS,
+  COMPLETE
+};
+
+volatile VM_Creation_State vm_created = NOT_CREATED;
+
 // 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;
@@ -3541,7 +3548,7 @@ static jint JNI_CreateJavaVM_inner(JavaVM **vm, void **penv, void *args) {
   // We use Atomic::xchg rather than Atomic::add/dec since on some platforms
   // the add/dec implementations are dependent on whether we are running
   // on a multiprocessor Atomic::xchg does not have this problem.
-  if (Atomic::xchg(&vm_created, 1) == 1) {
+  if (Atomic::xchg(&vm_created, IN_PROGRESS) != NOT_CREATED) {
     return JNI_EEXIST;   // already created, or create attempt in progress
   }
 
@@ -3554,8 +3561,6 @@ static jint JNI_CreateJavaVM_inner(JavaVM **vm, void **penv, void *args) {
     return JNI_ERR;
   }
 
-  assert(vm_created == 1, "vm_created is true during the creation");
-
   /**
    * Certain errors during initialization are recoverable and do not
    * prevent this method from being called again at a later time
@@ -3572,9 +3577,11 @@ static jint JNI_CreateJavaVM_inner(JavaVM **vm, void **penv, void *args) {
   if (result == JNI_OK) {
     JavaThread *thread = JavaThread::current();
     assert(!thread->has_pending_exception(), "should have returned not OK");
-    /* thread is thread_in_vm here */
+    // thread is thread_in_vm here
     *vm = (JavaVM *)(&main_vm);
     *(JNIEnv**)penv = thread->jni_environment();
+    // mark creation complete for other JNI ops
+    Atomic::release_store(&vm_created, COMPLETE);
 
 #if INCLUDE_JVMCI
     if (EnableJVMCI) {
@@ -3637,7 +3644,8 @@ static jint JNI_CreateJavaVM_inner(JavaVM **vm, void **penv, void *args) {
     *(JNIEnv**)penv = 0;
     // reset vm_created last to avoid race condition. Use OrderAccess to
     // control both compiler and architectural-based reordering.
-    Atomic::release_store(&vm_created, 0);
+    assert(vm_created == IN_PROGRESS, "must be");
+    Atomic::release_store(&vm_created, NOT_CREATED);
   }
 
   // Flush stdout and stderr before exit.
@@ -3666,7 +3674,7 @@ _JNI_IMPORT_OR_EXPORT_ jint JNICALL JNI_CreateJavaVM(JavaVM **vm, void **penv, v
 _JNI_IMPORT_OR_EXPORT_ jint JNICALL JNI_GetCreatedJavaVMs(JavaVM **vm_buf, jsize bufLen, jsize *numVMs) {
   HOTSPOT_JNI_GETCREATEDJAVAVMS_ENTRY((void **) vm_buf, bufLen, (uintptr_t *) numVMs);
 
-  if (vm_created == 1) {
+  if (vm_created == COMPLETE) {
     if (numVMs != nullptr) *numVMs = 1;
     if (bufLen > 0)     *vm_buf = (JavaVM *)(&main_vm);
   } else {
@@ -3686,7 +3694,7 @@ static jint JNICALL jni_DestroyJavaVM_inner(JavaVM *vm) {
   jint res = JNI_ERR;
   DT_RETURN_MARK(DestroyJavaVM, jint, (const jint&)res);
 
-  if (vm_created == 0) {
+  if (vm_created != COMPLETE) {
     res = JNI_ERR;
     return res;
   }
@@ -3717,7 +3725,7 @@ static jint JNICALL jni_DestroyJavaVM_inner(JavaVM *vm) {
   ThreadStateTransition::transition_from_native(thread, _thread_in_vm);
   Threads::destroy_vm();
   // Don't bother restoring thread state, VM is gone.
-  vm_created = 0;
+  vm_created = NOT_CREATED;
   return JNI_OK;
 }
 
@@ -3851,7 +3859,8 @@ 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) {
+  if (vm_created != COMPLETE) {
+    // Not sure how we could possibly get here.
     HOTSPOT_JNI_ATTACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
     return JNI_ERR;
   }
@@ -3864,7 +3873,8 @@ 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) {
+  if (vm_created != COMPLETE) {
+    // Not sure how we could possibly get here.
     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_ERR);
     return JNI_ERR;
   }
@@ -3927,7 +3937,10 @@ jint JNICALL jni_GetEnv(JavaVM *vm, void **penv, jint version) {
   jint ret = JNI_ERR;
   DT_RETURN_MARK(GetEnv, jint, (const jint&)ret);
 
-  if (vm_created == 0) {
+  // We can be called by native libraries in the JDK during VM
+  // initialization, so only bail-out if something seems very wrong.
+  // Though how would we get here in that case?
+  if (vm_created == NOT_CREATED) {
     *penv = nullptr;
     ret = JNI_EDETACHED;
     return ret;
@@ -3978,7 +3991,8 @@ jint JNICALL jni_GetEnv(JavaVM *vm, void **penv, jint version) {
 
 jint JNICALL jni_AttachCurrentThreadAsDaemon(JavaVM *vm, void **penv, void *_args) {
   HOTSPOT_JNI_ATTACHCURRENTTHREADASDAEMON_ENTRY(vm, penv, _args);
-  if (vm_created == 0) {
+  if (vm_created != COMPLETE) {
+    // Not sure how we could possibly get here.
   HOTSPOT_JNI_ATTACHCURRENTTHREADASDAEMON_RETURN((uint32_t) JNI_ERR);
     return JNI_ERR;
   }

test/hotspot/jtreg/runtime/jni/getCreatedJavaVMs/TestGetCreatedJavaVMs.java

@@ -0,0 +1,42 @@
+/*
+ * Copyright (c) 2023, 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.
+ */
+
+/*
+ * @test
+ * @library /test/lib
+ * @requires os.family != "Windows"
+ * @run driver TestGetCreatedJavaVMs
+ */
+import jdk.test.lib.Utils;
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+
+public class TestGetCreatedJavaVMs {
+    public static void main(String args[]) throws Exception {
+        ProcessBuilder pb = ProcessTools.createNativeTestProcessBuilder("GetCreatedJavaVMs");
+        OutputAnalyzer output = ProcessTools.executeProcess(pb);
+        output.shouldHaveExitValue(0);
+        output.reportDiagnosticSummary();
+    }
+}

test/hotspot/jtreg/runtime/jni/getCreatedJavaVMs/exeGetCreatedJavaVMs.c

@@ -0,0 +1,104 @@
+/*
+ * Copyright (c) 2023, 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.
+ */
+
+/* This code tests concurrent creation of and then attach to a JVM.
+ * Two threads race to create the JVM, the loser then checks GetCreatedJavaVMs
+ * and attaches to the returned JVM. Prior to the fix this could crash as the
+ * JVM is not fully initialized.
+ */
+#include "jni.h"
+#include <string.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#define NUM_THREADS 2
+
+void *thread_runner(void *threadid) {
+  int tid;
+  tid = (int)(intptr_t)threadid;
+
+  JavaVM *vm;
+  JNIEnv *env = 0;
+
+  JavaVMInitArgs vm_args;
+  JavaVMOption options[0];
+  vm_args.version = JNI_VERSION_1_2;
+  vm_args.nOptions = 0;
+  vm_args.options = options;
+  vm_args.ignoreUnrecognized = JNI_FALSE;
+
+  printf("[%d] BEGIN JNI_CreateJavaVM\n", tid);
+  jint create_res = JNI_CreateJavaVM(&vm, (void **)&env, &vm_args);
+  printf("[%d] END JNI_CreateJavaVM\n", tid);
+
+  if (create_res != JNI_OK) {
+    printf("[%d] Error creating JVM: %d\n", tid, create_res);
+    if (create_res == JNI_EEXIST) {
+      jsize count;
+      printf("[%d] BEGIN JNI_GetCreatedJavaVMs\n", tid);
+      jint get_res = JNI_GetCreatedJavaVMs(&vm, 1, &count);
+      printf("[%d] END JNI_GetCreatedJavaVMs\n", tid);
+
+      if (get_res != JNI_OK) {
+        printf("[%d] Error obtaining created VMs: %d\n", tid, get_res);
+        pthread_exit(NULL);
+      } else {
+        printf("[%d] Obtained %d created VMs\n", tid, count);
+      }
+      if (count > 0) {
+        printf("[%d] BEGIN AttachCurrentThread\n", tid);
+        get_res = (*vm)->AttachCurrentThread(vm, (void **)&env, NULL);
+        printf("[%d] END AttachCurrentThread - %s\n", tid,
+               (get_res == JNI_OK ? "succeeded" : "failed"));
+        if (get_res == JNI_OK) {
+          (*vm)->DetachCurrentThread(vm);
+        }
+      }
+      pthread_exit(NULL);
+    } else {
+      pthread_exit(NULL);
+    }
+  } else {
+    printf("[%d] Created a JVM\n", tid);
+  }
+
+  pthread_exit(NULL);
+}
+
+int main (int argc, char* argv[]) {
+  pthread_t threads[NUM_THREADS];
+  for (int i = 0; i < NUM_THREADS; i++ ) {
+    printf("[*] Creating thread %d\n", i);
+    int status = pthread_create(&threads[i], NULL, thread_runner, (void *)(intptr_t)i);
+    if (status != 0) {
+      printf("[*] Error creating thread %d - %d\n", i, status);
+      exit(-1);
+    }
+  }
+  for (int i = 0; i < NUM_THREADS; i++ ) {
+    pthread_join(threads[i], NULL);
+  }
+  return 0;
+}