Fix issue with observing invalid thread state.
Threads have two references to their java.lang.Thread peers,
'tlsPtr_.opeer' and 'tlsPtr_.jpeer'. The opeer is a direct
mirror::Object* which is used while the thread is running. The jpeer
is a global jobject used during early thread startup. As part of
thread startup the new thread will delete and clear the 'jpeer' and
only use 'opeer' from then on. A minor (DCHECK'd) consistency
guarantee is that only one of these fields can be observed to be set
at a time. Using JNI function table replacement it is possible to
execute JNI functions just before the DeleteGlobalRef of the jpeer. If
one calls other thread functions (such as through GetThreadInfo) these
functions may DCHECK that jpeer is cleared. This would fail since
jpeer wasn't cleared until after the DeleteGlobalRef returns.
This fixes the bug by clearing the 'jpeer' field before calling
DeleteGlobalRef.
Test: ./test.py --host
Bug: 146170834
Change-Id: I7e7941912a69fad9e75bbb55643eee0fa5d8a47d
diff --git a/runtime/thread.cc b/runtime/thread.cc
index f4e2b65..a4bc8ee 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -585,6 +585,14 @@
InitTid();
}
+void Thread::DeleteJPeer(JNIEnv* env) {
+ // Make sure nothing can observe both opeer and jpeer set at the same time.
+ jobject old_jpeer = tlsPtr_.jpeer;
+ CHECK(old_jpeer != nullptr);
+ tlsPtr_.jpeer = nullptr;
+ env->DeleteGlobalRef(old_jpeer);
+}
+
void* Thread::CreateCallback(void* arg) {
Thread* self = reinterpret_cast<Thread*>(arg);
Runtime* runtime = Runtime::Current();
@@ -614,8 +622,8 @@
// Copy peer into self, deleting global reference when done.
CHECK(self->tlsPtr_.jpeer != nullptr);
self->tlsPtr_.opeer = soa.Decode<mirror::Object>(self->tlsPtr_.jpeer).Ptr();
- self->GetJniEnv()->DeleteGlobalRef(self->tlsPtr_.jpeer);
- self->tlsPtr_.jpeer = nullptr;
+ // Make sure nothing can observe both opeer and jpeer set at the same time.
+ self->DeleteJPeer(self->GetJniEnv());
self->SetThreadName(self->GetThreadName()->ToModifiedUtf8().c_str());
ArtField* priorityField = jni::DecodeArtField(WellKnownClasses::java_lang_Thread_priority);
@@ -892,9 +900,9 @@
MutexLock mu(self, *Locks::runtime_shutdown_lock_);
runtime->EndThreadBirth();
}
- // Manually delete the global reference since Thread::Init will not have been run.
- env->DeleteGlobalRef(child_thread->tlsPtr_.jpeer);
- child_thread->tlsPtr_.jpeer = nullptr;
+ // Manually delete the global reference since Thread::Init will not have been run. Make sure
+ // nothing can observe both opeer and jpeer set at the same time.
+ child_thread->DeleteJPeer(env);
delete child_thread;
child_thread = nullptr;
// TODO: remove from thread group?
diff --git a/runtime/thread.h b/runtime/thread.h
index 9d703cf..0f6b369 100644
--- a/runtime/thread.h
+++ b/runtime/thread.h
@@ -1361,6 +1361,10 @@
~Thread() REQUIRES(!Locks::mutator_lock_, !Locks::thread_suspend_count_lock_);
void Destroy();
+ // Deletes and clears the tlsPtr_.jpeer field. Done in a way so that both it and opeer cannot be
+ // observed to be set at the same time by instrumentation.
+ void DeleteJPeer(JNIEnv* env);
+
void NotifyInTheadList()
REQUIRES_SHARED(Locks::thread_list_lock_);
diff --git a/test/928-jni-table/info.txt b/test/928-jni-table/info.txt
index 875a5f6..a5da6eb 100644
--- a/test/928-jni-table/info.txt
+++ b/test/928-jni-table/info.txt
@@ -1 +1,5 @@
Tests basic functions in the jvmti plugin.
+
+Tests that we can do basic things by replacing the JNIEnv vtable.
+
+TODO: We should really do more with this test.
diff --git a/test/928-jni-table/jni_table.cc b/test/928-jni-table/jni_table.cc
index 9a8b7fe..1dfe34b 100644
--- a/test/928-jni-table/jni_table.cc
+++ b/test/928-jni-table/jni_table.cc
@@ -39,6 +39,25 @@
return gOriginalEnv->NewGlobalRef(env, o);
}
+static void DoDeleteGlobalRef(JNIEnv* env, jobject o) {
+ jclass thr = env->FindClass("java/lang/Thread");
+ CHECK(thr != nullptr);
+ if (env->IsInstanceOf(o, thr)) {
+ jvmtiThreadInfo jti;
+ // b/146170834: This could cause DCHECK failures.
+ CHECK_EQ(jvmti_env->GetThreadInfo(reinterpret_cast<jthread>(o), &jti), JVMTI_ERROR_NONE);
+ }
+ gOriginalEnv->DeleteGlobalRef(env, o);
+}
+
+extern "C" JNIEXPORT void JNICALL Java_art_Test928_doOtherThreadTest(JNIEnv* env, jclass klass) {
+ size_t start_other_thread_count = gGlobalRefCount;
+ // Make sure it still works even on another thread.
+ jobject global = env->NewGlobalRef(klass);
+ CHECK_EQ(start_other_thread_count + 1, gGlobalRefCount);
+ env->DeleteGlobalRef(global);
+}
+
extern "C" JNIEXPORT void JNICALL Java_art_Test928_doJNITableTest(
JNIEnv* env, jclass klass) {
// Get the current table, as the delegate.
@@ -55,6 +74,7 @@
}
env_override->NewGlobalRef = CountNewGlobalRef;
+ env_override->DeleteGlobalRef = DoDeleteGlobalRef;
gGlobalRefCount = 0;
// Install the override.
@@ -67,14 +87,21 @@
CHECK_EQ(1u, gGlobalRefCount);
env->DeleteGlobalRef(global);
+ // Try and create and destroy a thread.
+ env->CallStaticVoidMethod(klass, env->GetStaticMethodID(klass, "runThreadTest", "()V"));
+ // Make sure something got ref'd, in the other thread we make and then clear a global ref so that
+ // should at least be present.
+ CHECK_LT(1u, gGlobalRefCount);
+
// Install the "original." There is no real reset.
+ size_t final_global_ref_count = gGlobalRefCount;
jvmtiError setoverride2_result = jvmti_env->SetJNIFunctionTable(gOriginalEnv);
if (JvmtiErrorToException(env, jvmti_env, setoverride2_result)) {
return;
}
jobject global2 = env->NewGlobalRef(klass);
- CHECK_EQ(1u, gGlobalRefCount);
+ CHECK_EQ(final_global_ref_count, gGlobalRefCount);
env->DeleteGlobalRef(global2);
// Try to install null. Should return NULL_POINTER error.
diff --git a/test/928-jni-table/src/art/Test928.java b/test/928-jni-table/src/art/Test928.java
index 0fbfb7e..1bea2a5 100644
--- a/test/928-jni-table/src/art/Test928.java
+++ b/test/928-jni-table/src/art/Test928.java
@@ -23,5 +23,15 @@
System.out.println("Done");
}
+ // Called by native code.
+ public static void runThreadTest() throws Exception {
+ Thread t = new Thread(() -> {
+ doOtherThreadTest();
+ });
+ t.start();
+ t.join();
+ }
+
public static native void doJNITableTest();
+ public static native void doOtherThreadTest();
}