From 0d146361f27e1415fab9272de1cdde84c074c718 Mon Sep 17 00:00:00 2001 From: Stefan Karlsson Date: Thu, 30 Nov 2023 09:46:26 +0000 Subject: [PATCH] 8320515: assert(monitor->object_peek() != nullptr) failed: Owned monitors should not have a dead object Reviewed-by: dholmes, ihse, sspitsyn, dcubed --- make/test/JtregNativeHotspot.gmk | 3 +- src/hotspot/share/prims/jvmtiEnvBase.cpp | 7 + src/hotspot/share/runtime/synchronizer.cpp | 1 - src/hotspot/share/runtime/vmOperations.cpp | 6 + .../Monitor/MonitorWithDeadObjectTest.java | 99 ++++++++++ .../Monitor/libMonitorWithDeadObjectTest.c | 172 ++++++++++++++++++ .../GetOwnedMonitorInfoTest.java | 37 +++- .../libGetOwnedMonitorInfoTest.c | 9 + 8 files changed, 326 insertions(+), 8 deletions(-) create mode 100644 test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java create mode 100644 test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c diff --git a/make/test/JtregNativeHotspot.gmk b/make/test/JtregNativeHotspot.gmk index 24fcac2198e..98101ee1e86 100644 --- a/make/test/JtregNativeHotspot.gmk +++ b/make/test/JtregNativeHotspot.gmk @@ -861,7 +861,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 exeGetCreatedJavaVMs.c + BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libMonitorWithDeadObjectTest.c libTestPsig.c exeGetCreatedJavaVMs.c BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit := jvm.lib BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack := jvm.lib BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exedaemonDestroy := jvm.lib @@ -1503,6 +1503,7 @@ else BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libterminatedThread += -lpthread BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit += -ljvm BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libCompleteExit += -lpthread + BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libMonitorWithDeadObjectTest += -lpthread BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libnativeStack += -lpthread BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exeGetCreatedJavaVMs := -ljvm -lpthread diff --git a/src/hotspot/share/prims/jvmtiEnvBase.cpp b/src/hotspot/share/prims/jvmtiEnvBase.cpp index 709cc90d561..9d6296ee316 100644 --- a/src/hotspot/share/prims/jvmtiEnvBase.cpp +++ b/src/hotspot/share/prims/jvmtiEnvBase.cpp @@ -2243,6 +2243,13 @@ JvmtiMonitorClosure::do_monitor(ObjectMonitor* mon) { } // Filter out on stack monitors collected during stack walk. oop obj = mon->object(); + + if (obj == nullptr) { + // This can happen if JNI code drops all references to the + // owning object. + return; + } + bool found = false; for (int j = 0; j < _owned_monitors_list->length(); j++) { jobject jobj = ((jvmtiMonitorStackDepthInfo*)_owned_monitors_list->at(j))->monitor; diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp index 4d01037380d..7c41010fdb8 100644 --- a/src/hotspot/share/runtime/synchronizer.cpp +++ b/src/hotspot/share/runtime/synchronizer.cpp @@ -1115,7 +1115,6 @@ void ObjectSynchronizer::owned_monitors_iterate_filtered(MonitorClosure* closure // ObjectMonitor cannot be async deflated. if (monitor->has_owner() && filter(monitor->owner_raw())) { assert(!monitor->is_being_async_deflated(), "Owned monitors should not be deflating"); - assert(monitor->object_peek() != nullptr, "Owned monitors should not have a dead object"); closure->do_monitor(monitor); } diff --git a/src/hotspot/share/runtime/vmOperations.cpp b/src/hotspot/share/runtime/vmOperations.cpp index cad9dd2ad54..3469e3f2cc0 100644 --- a/src/hotspot/share/runtime/vmOperations.cpp +++ b/src/hotspot/share/runtime/vmOperations.cpp @@ -347,6 +347,12 @@ class ObjectMonitorsDump : public MonitorClosure, public ObjectMonitorsView { return; } + if (monitor->object_peek() == nullptr) { + // JNI code doesn't necessarily keep the monitor object + // alive. Filter out monitors with dead objects. + return; + } + add(monitor); } diff --git a/test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java b/test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java new file mode 100644 index 00000000000..31aeaccf07a --- /dev/null +++ b/test/hotspot/jtreg/runtime/Monitor/MonitorWithDeadObjectTest.java @@ -0,0 +1,99 @@ +/* + * Copyright (c) 2022, 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. + * + */ + +/* + * @bug 8320515 + * @summary This test checks that ObjectMonitors with dead objects don't + * cause asserts, crashes, or failures when various sub-systems + * in the JVM find them. + * @library /testlibrary /test/lib + * @modules jdk.management + */ + +/* + * @requires os.family != "windows" & os.family != "aix" + * @test id=DetachThread + * @run main/othervm/native MonitorWithDeadObjectTest 0 + */ + +/* + * @requires os.family != "windows" & os.family != "aix" + * @test id=DumpThreadsBeforeDetach + * @run main/othervm/native MonitorWithDeadObjectTest 1 + */ + +/* + * @requires os.family != "windows" & os.family != "aix" + * @test id=DumpThreadsAfterDetach + * @run main/othervm/native MonitorWithDeadObjectTest 2 + */ + +import java.lang.management.ManagementFactory; +import java.lang.management.ThreadMXBean; + +public class MonitorWithDeadObjectTest { + public static native void createMonitorWithDeadObject(); + public static native void createMonitorWithDeadObjectDumpThreadsBeforeDetach(); + + static { + System.loadLibrary("MonitorWithDeadObjectTest"); + } + + private static void dumpThreadsWithLockedMonitors() { + ThreadMXBean threadBean = ManagementFactory.getThreadMXBean(); + threadBean.dumpAllThreads(true, false); + } + + private static void testDetachThread() { + // Create an ObjectMonitor with a dead object from an attached thread. + // This used to provoke an assert in DetachCurrentThread. + createMonitorWithDeadObject(); + } + + private static void testDumpThreadsBeforeDetach() { + // Create an ObjectMonitor with a dead object from an attached thread + // and perform a thread dump before detaching the thread. + createMonitorWithDeadObjectDumpThreadsBeforeDetach(); + } + + private static void testDumpThreadsAfterDetach() { + createMonitorWithDeadObject(); + + // The thread dumping code used to not tolerate monitors with dead + // objects and the detach code used to not unlock these monitors, so + // test that we don't end up with a bug where these monitors are not + // unlocked and then passed to the thread dumping code. + dumpThreadsWithLockedMonitors(); + } + + public static void main(String[] args) throws Exception { + int test = Integer.parseInt(args[0]); + switch (test) { + case 0: testDetachThread(); break; + case 1: testDumpThreadsBeforeDetach(); break; + case 2: testDumpThreadsAfterDetach(); break; + default: throw new RuntimeException("Unknown test"); + }; + } +} diff --git a/test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c b/test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c new file mode 100644 index 00000000000..7e12eac2ea2 --- /dev/null +++ b/test/hotspot/jtreg/runtime/Monitor/libMonitorWithDeadObjectTest.c @@ -0,0 +1,172 @@ +/* + * Copyright (c) 2022, 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. + */ + +#include +#include +#include +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif + +static JavaVM* jvm; +static pthread_t attacher; + +#define die(x) do { printf("%s:%s\n",x , __func__); perror(x); exit(EXIT_FAILURE); } while (0) + +static void check_exception(JNIEnv* env, const char* msg) { + if ((*env)->ExceptionCheck(env)) { + fprintf(stderr, "Error: %s", msg); + exit(-1); + } +} + +#define check(env, what, msg) \ + check_exception((env), (msg)); \ + do { \ + if ((what) == 0) { \ + fprintf(stderr, #what "is null: %s", (msg)); \ + exit(-2); \ + } \ + } while (0) + +static jobject create_object(JNIEnv* env) { + jclass clazz = (*env)->FindClass(env, "java/lang/Object"); + check(env, clazz, "No class"); + + jmethodID constructor = (*env)->GetMethodID(env, clazz, "", "()V"); + check(env, constructor, "No constructor"); + + jobject obj = (*env)->NewObject(env, clazz, constructor); + check(env, constructor, "No object"); + + return obj; +} + +static void system_gc(JNIEnv* env) { + jclass clazz = (*env)->FindClass(env, "java/lang/System"); + check(env, clazz, "No class"); + + jmethodID method = (*env)->GetStaticMethodID(env, clazz, "gc", "()V"); + check(env, method, "No method"); + + (*env)->CallStaticVoidMethod(env, clazz, method); + check_exception(env, "Calling System.gc()"); +} + +static void thread_dump_with_locked_monitors(JNIEnv* env) { + jclass ManagementFactoryClass = (*env)->FindClass(env, "java/lang/management/ManagementFactory"); + check(env, ManagementFactoryClass, "No ManagementFactory class"); + + jmethodID getThreadMXBeanMethod = (*env)->GetStaticMethodID(env, ManagementFactoryClass, "getThreadMXBean", "()Ljava/lang/management/ThreadMXBean;"); + check(env, getThreadMXBeanMethod, "No getThreadMXBean method"); + + jobject threadBean = (*env)->CallStaticObjectMethod(env, ManagementFactoryClass, getThreadMXBeanMethod); + check(env, threadBean, "Calling getThreadMXBean()"); + + jclass ThreadMXBeanClass = (*env)->FindClass(env, "java/lang/management/ThreadMXBean"); + check(env, ThreadMXBeanClass, "No ThreadMXBean class"); + + jmethodID dumpAllThreadsMethod = (*env)->GetMethodID(env, ThreadMXBeanClass, "dumpAllThreads", "(ZZ)[Ljava/lang/management/ThreadInfo;"); + check(env, dumpAllThreadsMethod, "No dumpAllThreads method"); + + // The 'lockedMonitors == true' is what causes the monitor with a dead object to be examined. + jobject array = (*env)->CallObjectMethod(env, threadBean, dumpAllThreadsMethod, JNI_TRUE /* lockedMonitors */, JNI_FALSE /* lockedSynchronizers*/); + check(env, array, "Calling dumpAllThreads(true, false)"); +} + +static void create_monitor_with_dead_object(JNIEnv* env) { + jobject obj = create_object(env); + + if ((*env)->MonitorEnter(env, obj) != 0) die("MonitorEnter"); + + // Drop the last strong reference to the object associated with the monitor. + // The monitor only keeps a weak reference to the object. + (*env)->DeleteLocalRef(env, obj); + + // Let the GC clear the weak reference to the object. + system_gc(env); +} + +static void* create_monitor_with_dead_object_in_thread(void* arg) { + JNIEnv* env; + int res = (*jvm)->AttachCurrentThread(jvm, (void**)&env, NULL); + if (res != JNI_OK) die("AttachCurrentThread"); + + // Make the correct incantation to create a monitor with a dead object. + create_monitor_with_dead_object(env); + + // DetachCurrentThread will try to unlock held monitors. This has been a + // source of at least two bugs: + // - When the object reference in the monitor was cleared, the monitor + // iterator code would skip it, preventing it from being unlocked when + // the owner thread detached, leaving it lingering in the system. + // - When the monitor iterator API was rewritten the code was changed to + // assert that we didn't have "owned" monitors with dead objects. This + // test provokes that situation and that asserts. + if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread"); + + return NULL; +} + +static void* create_monitor_with_dead_object_and_dump_threads_in_thread(void* arg) { + JNIEnv* env; + int res = (*jvm)->AttachCurrentThread(jvm, (void**)&env, NULL); + if (res != JNI_OK) die("AttachCurrentThread"); + + // Make the correct incantation to create a monitor with a dead object. + create_monitor_with_dead_object(env); + + // Perform a thread dump that checks for all thread's monitors. + // That code didn't expect the monitor iterators to return monitors + // with dead objects and therefore asserted/crashed. + thread_dump_with_locked_monitors(env); + + if ((*jvm)->DetachCurrentThread(jvm) != JNI_OK) die("DetachCurrentThread"); + + return NULL; +} + +JNIEXPORT void JNICALL Java_MonitorWithDeadObjectTest_createMonitorWithDeadObject(JNIEnv* env, jclass jc) { + void* ret; + + (*env)->GetJavaVM(env, &jvm); + + if (pthread_create(&attacher, NULL, create_monitor_with_dead_object_in_thread, NULL) != 0) die("pthread_create"); + if (pthread_join(attacher, &ret) != 0) die("pthread_join"); +} + +JNIEXPORT void JNICALL Java_MonitorWithDeadObjectTest_createMonitorWithDeadObjectDumpThreadsBeforeDetach(JNIEnv* env, jclass jc) { + void* ret; + + (*env)->GetJavaVM(env, &jvm); + + if (pthread_create(&attacher, NULL, create_monitor_with_dead_object_and_dump_threads_in_thread, NULL) != 0) die("pthread_create"); + if (pthread_join(attacher, &ret) != 0) die("pthread_join"); +} + +#ifdef __cplusplus +} +#endif diff --git a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java index 7d40301222c..7b99104670f 100644 --- a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java +++ b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/GetOwnedMonitorInfoTest.java @@ -24,8 +24,10 @@ /** * @test - * @bug 8185164 - * @summary Checks that a contended monitor does not show up in the list of owned monitors + * @bug 8185164 8320515 + * @summary Checks that a contended monitor does not show up in the list of owned monitors. + * 8320515 piggy-backs on this test and injects an owned monitor with a dead object, + and checks that that monitor isn't exposed to GetOwnedMonitorInfo. * @requires vm.jvmti * @compile GetOwnedMonitorInfoTest.java * @run main/othervm/native -agentlib:GetOwnedMonitorInfoTest GetOwnedMonitorInfoTest @@ -46,19 +48,42 @@ public class GetOwnedMonitorInfoTest { } } + private static native void jniMonitorEnter(Object obj); private static native int check(); private static native boolean hasEventPosted(); - public static void main(String[] args) throws Exception { - runTest(true); - runTest(false); + private static void jniMonitorEnterAndLetObjectDie() { + // The monitor iterator used by GetOwnedMonitorInfo used to + // assert when an owned monitor with a dead object was found. + // Inject this situation into this test that performs other + // GetOwnedMonitorInfo testing. + Object obj = new Object() {}; + jniMonitorEnter(obj); + if (!Thread.holdsLock(obj)) { + throw new RuntimeException("The object is not locked"); + } + obj = null; + System.gc(); } - public static void runTest(boolean isVirtual) throws Exception { + public static void main(String[] args) throws Exception { + runTest(true, true); + runTest(true, false); + runTest(false, true); + runTest(false, false); + } + + public static void runTest(boolean isVirtual, boolean jni) throws Exception { var threadFactory = isVirtual ? Thread.ofVirtual().factory() : Thread.ofPlatform().factory(); final GetOwnedMonitorInfoTest lock = new GetOwnedMonitorInfoTest(); Thread t1 = threadFactory.newThread(() -> { + Thread.currentThread().setName("Worker-Thread"); + + if (jni) { + jniMonitorEnterAndLetObjectDie(); + } + synchronized (lock) { System.out.println("Thread in sync section: " + Thread.currentThread().getName()); diff --git a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c index 5147fae5304..af4fd51f3b3 100644 --- a/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c +++ b/test/hotspot/jtreg/serviceability/jvmti/GetOwnedMonitorInfo/libGetOwnedMonitorInfoTest.c @@ -22,6 +22,7 @@ */ #include +#include #include #include "jvmti.h" #include "jni.h" @@ -264,6 +265,14 @@ jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) { return JNI_OK; } +JNIEXPORT void JNICALL +Java_GetOwnedMonitorInfoTest_jniMonitorEnter(JNIEnv* env, jclass cls, jobject obj) { + if ((*env)->MonitorEnter(env, obj) != 0) { + fprintf(stderr, "MonitorEnter failed"); + exit(-1); + } +} + JNIEXPORT jint JNICALL Java_GetOwnedMonitorInfoTest_check(JNIEnv *env, jclass cls) { return status;