Prevent races with GC when transferring objects between threads

We could have races with the GC when JVMTI code transfers a local
reference from one thread to another. This race would happen if a GC
was currently underway causing an unmoved reference to be transferred
to a thread that has already fixed up all of its references. This
meant that the receiving thread would now have a reference belonging
to the from-space on its stack.

This could cause memory errors and CHECK failures.

To fix this we make sure to run all checkpoints where local references
might be transferred in a GC-critical-section.

We also needed to fix a too-strict check in the method verifier where
we were incorrectly asserting that suspension be allowable when in
fact it was not necessary in all cases.

Test: ./test.py --host -j50
Bug: 67838964

Change-Id: Ib9166cc233d3c1b488864c0aff33246a3f99e436

8 files changed, 54 insertions, 14 deletions

diff --git a/openjdkjvmti/ti_method.cc b/openjdkjvmti/ti_method.cc
index 41679daaa1..fccc0b1197 100644
--- a/openjdkjvmti/ti_method.cc
+++ b/openjdkjvmti/ti_method.cc
@@ -788,7 +788,7 @@ jvmtiError MethodUtil::GetLocalVariableGeneric(jvmtiEnv* env ATTRIBUTE_UNUSED,
   }
   GetLocalVariableClosure c(self, depth, slot, type, val);
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!target->RequestSynchronousCheckpoint(&c)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(target, &c)) {
     return ERR(THREAD_NOT_ALIVE);
   } else {
     return c.GetResult();
@@ -917,7 +917,7 @@ jvmtiError MethodUtil::SetLocalVariableGeneric(jvmtiEnv* env ATTRIBUTE_UNUSED,
   }
   SetLocalVariableClosure c(self, depth, slot, type, val);
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!target->RequestSynchronousCheckpoint(&c)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(target, &c)) {
     return ERR(THREAD_NOT_ALIVE);
   } else {
     return c.GetResult();
@@ -976,7 +976,7 @@ jvmtiError MethodUtil::GetLocalInstance(jvmtiEnv* env ATTRIBUTE_UNUSED,
   }
   GetLocalInstanceClosure c(self, depth, data);
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!target->RequestSynchronousCheckpoint(&c)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(target, &c)) {
     return ERR(THREAD_NOT_ALIVE);
   } else {
     return c.GetResult();
diff --git a/openjdkjvmti/ti_monitor.cc b/openjdkjvmti/ti_monitor.cc
index 5881f8c7a9..7db0566a2e 100644
--- a/openjdkjvmti/ti_monitor.cc
+++ b/openjdkjvmti/ti_monitor.cc
@@ -395,7 +395,7 @@ jvmtiError MonitorUtil::GetCurrentContendedMonitor(jvmtiEnv* env ATTRIBUTE_UNUSE
   };
   GetContendedMonitorClosure closure(self, monitor);
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!target->RequestSynchronousCheckpoint(&closure)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(target, &closure)) {
     return ERR(THREAD_NOT_ALIVE);
   }
   return OK;
diff --git a/openjdkjvmti/ti_stack.cc b/openjdkjvmti/ti_stack.cc
index e346e16f92..b43eaa0286 100644
--- a/openjdkjvmti/ti_stack.cc
+++ b/openjdkjvmti/ti_stack.cc
@@ -257,7 +257,7 @@ jvmtiError StackUtil::GetStackTrace(jvmtiEnv* jvmti_env ATTRIBUTE_UNUSED,
                                        static_cast<size_t>(start_depth),
                                        static_cast<size_t>(max_frame_count));
     // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-    if (!thread->RequestSynchronousCheckpoint(&closure)) {
+    if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(thread, &closure)) {
       return ERR(THREAD_NOT_ALIVE);
     }
     *count_ptr = static_cast<jint>(closure.index);
@@ -268,7 +268,7 @@ jvmtiError StackUtil::GetStackTrace(jvmtiEnv* jvmti_env ATTRIBUTE_UNUSED,
   } else {
     GetStackTraceVectorClosure closure(0, 0);
     // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-    if (!thread->RequestSynchronousCheckpoint(&closure)) {
+    if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(thread, &closure)) {
       return ERR(THREAD_NOT_ALIVE);
     }
 
@@ -712,7 +712,7 @@ jvmtiError StackUtil::GetFrameCount(jvmtiEnv* env ATTRIBUTE_UNUSED,
 
   GetFrameCountClosure closure;
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!thread->RequestSynchronousCheckpoint(&closure)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(thread, &closure)) {
     return ERR(THREAD_NOT_ALIVE);
   }
 
@@ -802,7 +802,7 @@ jvmtiError StackUtil::GetFrameLocation(jvmtiEnv* env ATTRIBUTE_UNUSED,
 
   GetLocationClosure closure(static_cast<size_t>(depth));
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (!thread->RequestSynchronousCheckpoint(&closure)) {
+  if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(thread, &closure)) {
     return ERR(THREAD_NOT_ALIVE);
   }
 
@@ -923,7 +923,7 @@ static jvmtiError GetOwnedMonitorInfoCommon(jthread thread, Fn handle_results) {
     if (target != self) {
       called_method = true;
       // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-      if (!target->RequestSynchronousCheckpoint(&closure)) {
+      if (!ThreadUtil::RequestGCSafeSynchronousCheckpoint(target, &closure)) {
         return ERR(THREAD_NOT_ALIVE);
       }
     } else {
diff --git a/openjdkjvmti/ti_thread.cc b/openjdkjvmti/ti_thread.cc
index 99dea540e5..6d075a6b7b 100644
--- a/openjdkjvmti/ti_thread.cc
+++ b/openjdkjvmti/ti_thread.cc
@@ -38,6 +38,9 @@
 #include "base/mutex.h"
 #include "events-inl.h"
 #include "gc/system_weak.h"
+#include "gc/collector_type.h"
+#include "gc/gc_cause.h"
+#include "gc/scoped_gc_critical_section.h"
 #include "gc_root-inl.h"
 #include "jni_internal.h"
 #include "mirror/class.h"
@@ -1061,7 +1064,7 @@ jvmtiError ThreadUtil::StopThread(jvmtiEnv* env ATTRIBUTE_UNUSED,
   };
   StopThreadClosure c(exc);
   // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution.
-  if (target->RequestSynchronousCheckpoint(&c)) {
+  if (RequestGCSafeSynchronousCheckpoint(target, &c)) {
     return OK;
   } else {
     // Something went wrong, probably the thread died.
@@ -1084,4 +1087,29 @@ jvmtiError ThreadUtil::InterruptThread(jvmtiEnv* env ATTRIBUTE_UNUSED, jthread t
   return OK;
 }
 
+class GcCriticalSectionClosure : public art::Closure {
+ public:
+  explicit GcCriticalSectionClosure(art::Closure* wrapped) : wrapped_(wrapped) {}
+
+  void Run(art::Thread* self) OVERRIDE {
+    if (art::kIsDebugBuild) {
+      art::Locks::thread_list_lock_->AssertNotHeld(art::Thread::Current());
+    }
+    // This might block as it waits for any in-progress GCs to finish but this is fine since we
+    // released the Thread-list-lock prior to calling this in RequestSynchronousCheckpoint.
+    art::gc::ScopedGCCriticalSection sgccs(art::Thread::Current(),
+                                           art::gc::kGcCauseDebugger,
+                                           art::gc::kCollectorTypeDebugger);
+    wrapped_->Run(self);
+  }
+
+ private:
+  art::Closure* wrapped_;
+};
+
+bool ThreadUtil::RequestGCSafeSynchronousCheckpoint(art::Thread* thr, art::Closure* function) {
+  GcCriticalSectionClosure gccsc(function);
+  return thr->RequestSynchronousCheckpoint(&gccsc);
+}
+
 }  // namespace openjdkjvmti
diff --git a/openjdkjvmti/ti_thread.h b/openjdkjvmti/ti_thread.h
index 09b4cabcfc..341bffe51e 100644
--- a/openjdkjvmti/ti_thread.h
+++ b/openjdkjvmti/ti_thread.h
@@ -42,6 +42,7 @@ namespace art {
 class ArtField;
 class ScopedObjectAccessAlreadyRunnable;
 class Thread;
+class Closure;
 }  // namespace art
 
 namespace openjdkjvmti {
@@ -133,6 +134,16 @@ class ThreadUtil {
     REQUIRES(!art::Locks::user_code_suspension_lock_,
              !art::Locks::thread_suspend_count_lock_);
 
+  // This will request a synchronous checkpoint in such a way as to prevent gc races if a local
+  // variable is taken from one thread's stack and placed in the stack of another thread.
+  // RequestSynchronousCheckpoint releases the thread_list_lock_ as a part of its execution. This is
+  // due to the fact that Thread::Current() needs to go to sleep to allow the targeted thread to
+  // execute the checkpoint for us if it is Runnable.
+  static bool RequestGCSafeSynchronousCheckpoint(art::Thread* thr, art::Closure* function)
+      REQUIRES_SHARED(art::Locks::mutator_lock_)
+      RELEASE(art::Locks::thread_list_lock_)
+      REQUIRES(!art::Locks::thread_suspend_count_lock_);
+
  private:
   // We need to make sure only one thread tries to suspend threads at a time so we can get the
   // 'suspend-only-once' behavior the spec requires. Internally, ART considers suspension to be a
diff --git a/runtime/verifier/method_verifier.cc b/runtime/verifier/method_verifier.cc
index 0033167160..cdea0e5e02 100644
--- a/runtime/verifier/method_verifier.cc
+++ b/runtime/verifier/method_verifier.cc
@@ -553,7 +553,7 @@ MethodVerifier::MethodVerifier(Thread* self,
     : self_(self),
       arena_stack_(Runtime::Current()->GetArenaPool()),
       allocator_(&arena_stack_),
-      reg_types_(can_load_classes, allocator_),
+      reg_types_(can_load_classes, allocator_, allow_thread_suspension),
       reg_table_(allocator_),
       work_insn_idx_(dex::kDexNoIndex),
       dex_method_idx_(dex_method_idx),
diff --git a/runtime/verifier/reg_type_cache.cc b/runtime/verifier/reg_type_cache.cc
index 4ebe151f76..0029eb90a3 100644
--- a/runtime/verifier/reg_type_cache.cc
+++ b/runtime/verifier/reg_type_cache.cc
@@ -268,12 +268,13 @@ const RegType& RegTypeCache::FromClass(const char* descriptor, mirror::Class* kl
   return *reg_type;
 }
 
-RegTypeCache::RegTypeCache(bool can_load_classes, ScopedArenaAllocator& allocator)
+RegTypeCache::RegTypeCache(bool can_load_classes, ScopedArenaAllocator& allocator, bool can_suspend)
     : entries_(allocator.Adapter(kArenaAllocVerifier)),
       klass_entries_(allocator.Adapter(kArenaAllocVerifier)),
       can_load_classes_(can_load_classes),
       allocator_(allocator) {
-  if (kIsDebugBuild) {
+  DCHECK(can_suspend || !can_load_classes) << "Cannot load classes is suspension is disabled!";
+  if (kIsDebugBuild && can_suspend) {
     Thread::Current()->AssertThreadSuspensionIsAllowable(gAborting == 0);
   }
   // The klass_entries_ array does not have primitives or small constants.
diff --git a/runtime/verifier/reg_type_cache.h b/runtime/verifier/reg_type_cache.h
index 74d9e9de11..cb16b15054 100644
--- a/runtime/verifier/reg_type_cache.h
+++ b/runtime/verifier/reg_type_cache.h
@@ -61,7 +61,7 @@ static constexpr size_t kDefaultArenaBitVectorBytes = 8;
 
 class RegTypeCache {
  public:
-  explicit RegTypeCache(bool can_load_classes, ScopedArenaAllocator& allocator);
+  RegTypeCache(bool can_load_classes, ScopedArenaAllocator& allocator, bool can_suspend = true);
   ~RegTypeCache();
   static void Init() REQUIRES_SHARED(Locks::mutator_lock_) {
     if (!RegTypeCache::primitive_initialized_) {