Skip to content

Commit

Permalink
8308341: JNI_GetCreatedJavaVMs returns a partially initialized JVM
Browse files Browse the repository at this point in the history
Reviewed-by: jsjolen, gziemski
  • Loading branch information
David Holmes committed May 30, 2023
1 parent cb40db0 commit 1e6770f
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 15 deletions.
5 changes: 3 additions & 2 deletions make/test/JtregNativeHotspot.gmk
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
40 changes: 27 additions & 13 deletions src/hotspot/share/prims/jni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
}

Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -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;
}

1 comment on commit 1e6770f

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.