Perform SetEvent under the user_code_suspend_count_lock_
We would perform major parts of SetEvent without any synchronization
and perform multiple internal suspensions. This could lead to
inconsistent state or deadlocks.
Test: ./test.py --host
Test: ./art/tools/run-libjdwp-tests.sh --mode=host
Change-Id: Ice2fc94b4f82fcd5938928e8dbf1bdddd29000ab
diff --git a/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index 2c5ebfe..beb864b 100644
--- a/openjdkjvmti/events.cc
+++ b/openjdkjvmti/events.cc
@@ -38,6 +38,7 @@
#include "art_field-inl.h"
#include "art_jvmti.h"
#include "art_method-inl.h"
+#include "base/mutex.h"
#include "deopt_manager.h"
#include "dex/dex_file_types.h"
#include "gc/allocation_listener.h"
@@ -1267,57 +1268,65 @@
return ERR(MUST_POSSESS_CAPABILITY);
}
- art::Thread* art_thread = nullptr;
- if (thread != nullptr) {
- if (!IsThreadControllable(event)) {
- return ERR(ILLEGAL_ARGUMENT);
- }
- art::ScopedObjectAccess soa(art::Thread::Current());
- art::MutexLock mu(soa.Self(), *art::Locks::thread_list_lock_);
- jvmtiError err = ERR(INTERNAL);
- if (!ThreadUtil::GetAliveNativeThread(thread, soa, &art_thread, &err)) {
- return err;
- } else if (art_thread->IsStillStarting()) {
- return ERR(THREAD_NOT_ALIVE);
- }
- art::ThreadState state = art_thread->GetState();
- if (state == art::ThreadState::kStarting || state == art::ThreadState::kTerminated) {
- return ERR(THREAD_NOT_ALIVE);
- }
+ if (thread != nullptr && !IsThreadControllable(event)) {
+ return ERR(ILLEGAL_ARGUMENT);
}
- // TODO We use art_thread simply as a global unique identifier here. It is not safe to actually
- // use it without holding the thread_list_lock_.
-
- bool old_state;
- bool new_state;
-
- {
- // Change the event masks atomically.
- art::Thread* self = art::Thread::Current();
- art::WriterMutexLock mu(self, envs_lock_);
- art::WriterMutexLock mu_env_info(self, env->event_info_mutex_);
- old_state = global_mask.Test(event);
- if (mode == JVMTI_ENABLE) {
- env->event_masks.EnableEvent(env, art_thread, event);
- global_mask.Set(event);
- new_state = true;
- } else {
- DCHECK_EQ(mode, JVMTI_DISABLE);
-
- // TODO Replace art_thread with a uintptr_t or something to indicate we cannot read from it.
- env->event_masks.DisableEvent(env, art_thread, event);
- RecalculateGlobalEventMaskLocked(event);
- new_state = global_mask.Test(event);
+ art::Thread* self = art::Thread::Current();
+ art::Thread* target = nullptr;
+ do {
+ ThreadUtil::SuspendCheck(self);
+ art::MutexLock ucsl_mu(self, *art::Locks::user_code_suspension_lock_);
+ // Make sure we won't be suspended in the middle of holding the
+ // thread_suspend_count_lock_ by a user-code suspension. We retry and do
+ // another SuspendCheck to clear this.
+ if (ThreadUtil::WouldSuspendForUserCodeLocked(self)) {
+ continue;
}
- }
+ bool old_state;
+ bool new_state;
+ {
+ // From now on we know we cannot get suspended by user-code.
+ // NB This does a SuspendCheck (during thread state change) so we need to
+ // make sure we don't have the 'suspend_lock' locked here.
+ art::ScopedObjectAccess soa(self);
+ art::WriterMutexLock el_mu(self, envs_lock_);
+ art::MutexLock tll_mu(self, *art::Locks::thread_list_lock_);
+ jvmtiError err = ERR(INTERNAL);
+ if (thread != nullptr) {
+ if (!ThreadUtil::GetAliveNativeThread(thread, soa, &target, &err)) {
+ return err;
+ } else if (target->IsStillStarting() ||
+ target->GetState() == art::ThreadState::kStarting) {
+ target->Dump(LOG_STREAM(WARNING) << "Is not alive: ");
+ return ERR(THREAD_NOT_ALIVE);
+ }
+ }
- // Handle any special work required for the event type.
- if (new_state != old_state) {
- return HandleEventType(event, thread, mode == JVMTI_ENABLE);
- }
- return ERR(NONE);
+ {
+ art::WriterMutexLock ei_mu(self, env->event_info_mutex_);
+ old_state = global_mask.Test(event);
+ if (mode == JVMTI_ENABLE) {
+ env->event_masks.EnableEvent(env, target, event);
+ global_mask.Set(event);
+ new_state = true;
+ } else {
+ DCHECK_EQ(mode, JVMTI_DISABLE);
+
+ env->event_masks.DisableEvent(env, target, event);
+ RecalculateGlobalEventMaskLocked(event);
+ new_state = global_mask.Test(event);
+ }
+ }
+ }
+ // Handle any special work required for the event type. We still have the
+ // user_code_suspend_count_lock_ so there won't be any interleaving here.
+ if (new_state != old_state) {
+ return HandleEventType(event, thread, mode == JVMTI_ENABLE);
+ }
+ return OK;
+ } while (true);
}
void EventHandler::HandleBreakpointEventsChanged(bool added) {
@@ -1340,7 +1349,7 @@
}
EventHandler::EventHandler()
- : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kTopLockLevel),
+ : envs_lock_("JVMTI Environment List Lock", art::LockLevel::kPostMutatorTopLockLevel),
frame_pop_enabled(false) {
alloc_listener_.reset(new JvmtiAllocationListener(this));
ddm_listener_.reset(new JvmtiDdmChunkListener(this));
diff --git a/openjdkjvmti/events.h b/openjdkjvmti/events.h
index e48772c..2c3c7a0 100644
--- a/openjdkjvmti/events.h
+++ b/openjdkjvmti/events.h
@@ -21,6 +21,7 @@
#include <vector>
#include <android-base/logging.h>
+#include <android-base/thread_annotations.h>
#include "base/macros.h"
#include "base/mutex.h"
@@ -323,9 +324,9 @@
// need to be able to remove arbitrary elements from it.
std::list<ArtJvmTiEnv*> envs GUARDED_BY(envs_lock_);
- // Top level lock. Nothing at all should be held when we lock this.
- mutable art::ReaderWriterMutex envs_lock_
- ACQUIRED_BEFORE(art::Locks::instrument_entrypoints_lock_);
+ // Close to top level lock. Nothing should be held when we lock this (except for mutator_lock_
+ // which is needed when setting new events).
+ mutable art::ReaderWriterMutex envs_lock_ ACQUIRED_AFTER(art::Locks::mutator_lock_);
// A union of all enabled events, anywhere.
EventMask global_mask;
diff --git a/runtime/base/locks.cc b/runtime/base/locks.cc
index a7922a2..4349be0 100644
--- a/runtime/base/locks.cc
+++ b/runtime/base/locks.cc
@@ -158,9 +158,9 @@
DCHECK(runtime_thread_pool_lock_ != nullptr);
} else {
// Create global locks in level order from highest lock level to lowest.
- LockLevel current_lock_level = kInstrumentEntrypointsLock;
- DCHECK(instrument_entrypoints_lock_ == nullptr);
- instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
+ LockLevel current_lock_level = kUserCodeSuspensionLock;
+ DCHECK(user_code_suspension_lock_ == nullptr);
+ user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
#define UPDATE_CURRENT_LOCK_LEVEL(new_level) \
if ((new_level) >= current_lock_level) { \
@@ -171,9 +171,9 @@
} \
current_lock_level = new_level;
- UPDATE_CURRENT_LOCK_LEVEL(kUserCodeSuspensionLock);
- DCHECK(user_code_suspension_lock_ == nullptr);
- user_code_suspension_lock_ = new Mutex("user code suspension lock", current_lock_level);
+ UPDATE_CURRENT_LOCK_LEVEL(kInstrumentEntrypointsLock);
+ DCHECK(instrument_entrypoints_lock_ == nullptr);
+ instrument_entrypoints_lock_ = new Mutex("instrument entrypoint lock", current_lock_level);
UPDATE_CURRENT_LOCK_LEVEL(kMutatorLock);
DCHECK(mutator_lock_ == nullptr);
diff --git a/runtime/base/locks.h b/runtime/base/locks.h
index b7d8e31..b15fd32 100644
--- a/runtime/base/locks.h
+++ b/runtime/base/locks.h
@@ -121,9 +121,14 @@
kRuntimeShutdownLock,
kTraceLock,
kHeapBitmapLock,
+
+ // This is a generic lock level for a top-level lock meant to be gained after having the
+ // mutator_lock_.
+ kPostMutatorTopLockLevel,
+
kMutatorLock,
- kUserCodeSuspensionLock,
kInstrumentEntrypointsLock,
+ kUserCodeSuspensionLock,
kZygoteCreationLock,
// The highest valid lock level. Use this if there is code that should only be called with no
@@ -174,13 +179,13 @@
// Check if the given mutex is in expected_mutexes_on_weak_ref_access_.
static bool IsExpectedOnWeakRefAccess(BaseMutex* mutex);
- // Guards allocation entrypoint instrumenting.
- static Mutex* instrument_entrypoints_lock_;
-
// Guards code that deals with user-code suspension. This mutex must be held when suspending or
// resuming threads with SuspendReason::kForUserCode. It may be held by a suspended thread, but
// only if the suspension is not due to SuspendReason::kForUserCode.
- static Mutex* user_code_suspension_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
+ static Mutex* user_code_suspension_lock_;
+
+ // Guards allocation entrypoint instrumenting.
+ static Mutex* instrument_entrypoints_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
// A barrier is used to synchronize the GC/Debugger thread with mutator threads. When GC/Debugger
// thread wants to suspend all mutator threads, it needs to wait for all mutator threads to pass
@@ -217,7 +222,7 @@
// state is changed | .. running ..
// - if the CAS operation fails then goto x | .. running ..
// .. running .. | .. running ..
- static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(user_code_suspension_lock_);
+ static MutatorMutex* mutator_lock_ ACQUIRED_AFTER(instrument_entrypoints_lock_);
// Allow reader-writer mutual exclusion on the mark and live bitmaps of the heap.
static ReaderWriterMutex* heap_bitmap_lock_ ACQUIRED_AFTER(mutator_lock_);