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

4 files changed, 76 insertions, 61 deletions

diff --git a/openjdkjvmti/events.cc b/openjdkjvmti/events.cc
index 2c5ebfe3e1..beb864be23 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 @@ jvmtiError EventHandler::SetEvent(ArtJvmTiEnv* env,
     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_.
+  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);
+        }
+      }
 
-  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);
+      {
+        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);
 
-      // 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);
+          env->event_masks.DisableEvent(env, target, event);
+          RecalculateGlobalEventMaskLocked(event);
+          new_state = global_mask.Test(event);
+        }
+      }
     }
-  }
-
-  // Handle any special work required for the event type.
-  if (new_state != old_state) {
-    return HandleEventType(event, thread, mode == JVMTI_ENABLE);
-  }
-
-  return ERR(NONE);
+    // 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 @@ void EventHandler::Shutdown() {
 }
 
 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 e48772cc66..2c3c7a0ba5 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 @@ class EventHandler {
   // 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 a7922a213c..4349be09b0 100644
--- a/runtime/base/locks.cc
+++ b/runtime/base/locks.cc
@@ -158,9 +158,9 @@ void Locks::Init() {
     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 @@ void Locks::Init() {
       } \
       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 b7d8e31dbf..b15fd32f4d 100644
--- a/runtime/base/locks.h
+++ b/runtime/base/locks.h
@@ -121,9 +121,14 @@ enum LockLevel : uint8_t {
   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 @@ class Locks {
   // 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 @@ class Locks {
   //    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_);