8233549: Thread interrupted state must only be accessed when not in a safepoint-safe state

Reviewed-by: dcubed, sspitsyn

src/hotspot/os/posix/os_posix.cpp

@@ -2075,10 +2075,12 @@ void Parker::park(bool isAbsolute, jlong time) {
   // the ThreadBlockInVM() CTOR and DTOR may grab Threads_lock.
   ThreadBlockInVM tbivm(jt);
 
+  // Can't access interrupt state now that we are _thread_blocked. If we've
+  // been interrupted since we checked above then _counter will be > 0.
+
   // Don't wait if cannot get lock since interference arises from
-  // unparking. Also re-check interrupt before trying wait.
-  if (jt->is_interrupted(false) ||
-      pthread_mutex_trylock(_mutex) != 0) {
+  // unparking.
+  if (pthread_mutex_trylock(_mutex) != 0) {
     return;
   }
 

src/hotspot/os/solaris/os_solaris.cpp

@@ -4925,10 +4925,12 @@ void Parker::park(bool isAbsolute, jlong time) {
   // the ThreadBlockInVM() CTOR and DTOR may grab Threads_lock.
   ThreadBlockInVM tbivm(jt);
 
+  // Can't access interrupt state now that we are _thread_blocked. If we've
+  // been interrupted since we checked above then _counter will be > 0.
+
   // Don't wait if cannot get lock since interference arises from
-  // unblocking.  Also. check interrupt before trying wait
-  if (jt->is_interrupted(false) ||
-      os::Solaris::mutex_trylock(_mutex) != 0) {
+  // unblocking.
+  if (os::Solaris::mutex_trylock(_mutex) != 0) {
     return;
   }
 

src/hotspot/share/classfile/javaClasses.cpp

@@ -1708,10 +1708,20 @@ void java_lang_Thread::set_thread(oop java_thread, JavaThread* thread) {
 }
 
 bool java_lang_Thread::interrupted(oop java_thread) {
+  // Make sure the caller can safely access oops.
+  assert(Thread::current()->is_VM_thread() ||
+         (JavaThread::current()->thread_state() != _thread_blocked &&
+          JavaThread::current()->thread_state() != _thread_in_native),
+         "Unsafe access to oop");
   return java_thread->bool_field_volatile(_interrupted_offset);
 }
 
 void java_lang_Thread::set_interrupted(oop java_thread, bool val) {
+  // Make sure the caller can safely access oops.
+  assert(Thread::current()->is_VM_thread() ||
+         (JavaThread::current()->thread_state() != _thread_blocked &&
+          JavaThread::current()->thread_state() != _thread_in_native),
+         "Unsafe access to oop");
   java_thread->bool_field_put_volatile(_interrupted_offset, val);
 }
 

src/hotspot/share/prims/jvmtiEnv.cpp

@@ -3333,35 +3333,8 @@ JvmtiEnv::RawMonitorExit(JvmtiRawMonitor * rmonitor) {
 // rmonitor - pre-checked for validity
 jvmtiError
 JvmtiEnv::RawMonitorWait(JvmtiRawMonitor * rmonitor, jlong millis) {
-  int r = 0;
   Thread* thread = Thread::current();
-
-  if (thread->is_Java_thread()) {
-    JavaThread* current_thread = (JavaThread*)thread;
-
-    /* Transition to thread_blocked without entering vm state          */
-    /* This is really evil. Normally you can't undo _thread_blocked    */
-    /* transitions like this because it would cause us to miss a       */
-    /* safepoint but since the thread was already in _thread_in_native */
-    /* the thread is not leaving a safepoint safe state and it will    */
-    /* block when it tries to return from native. We can't safepoint   */
-    /* block in here because we could deadlock the vmthread. Blech.    */
-
-    JavaThreadState state = current_thread->thread_state();
-    assert(state == _thread_in_native, "Must be _thread_in_native");
-    // frame should already be walkable since we are in native
-    assert(!current_thread->has_last_Java_frame() ||
-           current_thread->frame_anchor()->walkable(), "Must be walkable");
-    current_thread->set_thread_state(_thread_blocked);
-
-    r = rmonitor->raw_wait(millis, true, current_thread);
-    // restore state, still at a safepoint safe state
-    current_thread->set_thread_state(state);
-
-  } else {
-      r = rmonitor->raw_wait(millis, false, thread);
-      assert(r != JvmtiRawMonitor::M_INTERRUPTED, "non-JavaThread can't be interrupted");
-  }
+  int r = rmonitor->raw_wait(millis, thread);
 
   switch (r) {
   case JvmtiRawMonitor::M_INTERRUPTED:

src/hotspot/share/prims/jvmtiRawMonitor.cpp

@@ -174,29 +174,16 @@ void JvmtiRawMonitor::simple_exit(Thread* self) {
   return;
 }
 
-int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
-  guarantee(_owner == self  , "invariant");
-  guarantee(_recursions == 0, "invariant");
-
-  QNode node(self);
+inline void JvmtiRawMonitor::enqueue_waiter(QNode& node) {
   node._notified = 0;
   node._t_state = QNode::TS_WAIT;
-
   RawMonitor_lock->lock_without_safepoint_check();
   node._next = _wait_set;
   _wait_set = &node;
   RawMonitor_lock->unlock();
+}
 
-  simple_exit(self);
-  guarantee(_owner != self, "invariant");
-
-  int ret = OS_OK;
-  if (millis <= 0) {
-    self->_ParkEvent->park();
-  } else {
-    ret = self->_ParkEvent->park(millis);
-  }
-
+inline void JvmtiRawMonitor::dequeue_waiter(QNode& node) {
   // If thread still resides on the waitset then unlink it.
   // Double-checked locking -- the usage is safe in this context
   // as _t_state is volatile and the lock-unlock operators are
@@ -225,10 +212,60 @@ int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
   }
 
   guarantee(node._t_state == QNode::TS_RUN, "invariant");
-  simple_enter(self);
+}
 
+// simple_wait is not quite so simple as we have to deal with the interaction
+// with the Thread interrupt state, which resides in the java.lang.Thread object.
+// That state must only be accessed while _thread_in_vm and requires proper thread-state
+// transitions. However, we cannot perform such transitions whilst we hold the RawMonitor,
+// else we can deadlock with the VMThread (which may also use RawMonitors as part of
+// executing various callbacks).
+// Returns M_OK usually, but M_INTERRUPTED if the thread is a JavaThread and was
+// interrupted.
+int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
+  guarantee(_owner == self  , "invariant");
+  guarantee(_recursions == 0, "invariant");
+
+  QNode node(self);
+  enqueue_waiter(node);
+
+  simple_exit(self);
+  guarantee(_owner != self, "invariant");
+
+  int ret = M_OK;
+  if (self->is_Java_thread()) {
+    JavaThread* jt = (JavaThread*) self;
+    // Transition to VM so we can check interrupt state
+    ThreadInVMfromNative tivm(jt);
+    if (jt->is_interrupted(true)) {
+        ret = M_INTERRUPTED;
+    } else {
+      ThreadBlockInVM tbivm(jt);
+      jt->set_suspend_equivalent();
+      if (millis <= 0) {
+        self->_ParkEvent->park();
+      } else {
+        self->_ParkEvent->park(millis);
+      }
+      // Return to VM before post-check of interrupt state
+    }
+    if (jt->is_interrupted(true)) {
+      ret = M_INTERRUPTED;
+    }
+  } else {
+    if (millis <= 0) {
+      self->_ParkEvent->park();
+    } else {
+      self->_ParkEvent->park(millis);
+    }
+  }
+
+  dequeue_waiter(node);
+
+  simple_enter(self);
   guarantee(_owner == self, "invariant");
   guarantee(_recursions == 0, "invariant");
+
   return ret;
 }
 
@@ -351,60 +388,59 @@ int JvmtiRawMonitor::raw_exit(Thread* self) {
   return M_OK;
 }
 
-// All JavaThreads will enter here with state _thread_blocked
-
-int JvmtiRawMonitor::raw_wait(jlong millis, bool interruptible, Thread* self) {
+int JvmtiRawMonitor::raw_wait(jlong millis, Thread* self) {
   if (self != _owner) {
     return M_ILLEGAL_MONITOR_STATE;
   }
 
+  int ret = M_OK;
+
   // To avoid spurious wakeups we reset the parkevent. This is strictly optional.
   // The caller must be able to tolerate spurious returns from raw_wait().
   self->_ParkEvent->reset();
   OrderAccess::fence();
 
-  JavaThread* jt = NULL;
-  // check interrupt event
-  if (interruptible) {
-    assert(self->is_Java_thread(), "Only JavaThreads can be interruptible");
-    jt = (JavaThread*)self;
-    if (jt->is_interrupted(true)) {
-      return M_INTERRUPTED;
-    }
-  } else {
-    assert(!self->is_Java_thread(), "JavaThreads must be interuptible");
-  }
-
   intptr_t save = _recursions;
   _recursions = 0;
   _waiters++;
-  if (self->is_Java_thread()) {
-    guarantee(jt->thread_state() == _thread_blocked, "invariant");
-    jt->set_suspend_equivalent();
-  }
-  int rv = simple_wait(self, millis);
+  ret = simple_wait(self, millis);
   _recursions = save;
   _waiters--;
 
   guarantee(self == _owner, "invariant");
+
   if (self->is_Java_thread()) {
+    JavaThread* jt = (JavaThread*)self;
     for (;;) {
+      jt->set_suspend_equivalent();
       if (!jt->handle_special_suspend_equivalent_condition()) {
         break;
+      } else {
+        // We've been suspended whilst waiting and so we have to
+        // relinquish the raw monitor until we are resumed. Of course
+        // after reacquiring we have to re-check for suspension again.
+        // Suspension requires we are _thread_blocked, and we also have to
+        // recheck for being interrupted.
+        simple_exit(jt);
+        {
+          ThreadInVMfromNative tivm(jt);
+          {
+            ThreadBlockInVM tbivm(jt);
+            jt->java_suspend_self();
+          }
+          if (jt->is_interrupted(true)) {
+            ret = M_INTERRUPTED;
+          }
+        }
+        simple_enter(jt);
       }
-      simple_exit(jt);
-      jt->java_suspend_self();
-      simple_enter(jt);
-      jt->set_suspend_equivalent();
     }
     guarantee(jt == _owner, "invariant");
+  } else {
+    assert(ret != M_INTERRUPTED, "Only JavaThreads can be interrupted");
   }
 
-  if (interruptible && jt->is_interrupted(true)) {
-    return M_INTERRUPTED;
-  }
-
-  return M_OK;
+  return ret;
 }
 
 int JvmtiRawMonitor::raw_notify(Thread* self) {

src/hotspot/share/prims/jvmtiRawMonitor.hpp

@@ -65,6 +65,11 @@ class JvmtiRawMonitor : public CHeapObj<mtSynchronizer>  {
   // JVMTI_RM_MAGIC is set in contructor and unset in destructor.
   enum { JVMTI_RM_MAGIC = (int)(('T' << 24) | ('I' << 16) | ('R' << 8) | 'M') };
 
+  // Helpers for queue management isolation
+  void enqueue_waiter(QNode& node);
+  void dequeue_waiter(QNode& node);
+
+  // Mostly low-level implementation routines
   void simple_enter(Thread* self);
   void simple_exit(Thread* self);
   int simple_wait(Thread* self, jlong millis);
@@ -92,7 +97,7 @@ class JvmtiRawMonitor : public CHeapObj<mtSynchronizer>  {
   int recursions() const { return _recursions; }
   void raw_enter(Thread* self);
   int raw_exit(Thread* self);
-  int raw_wait(jlong millis, bool interruptible, Thread* self);
+  int raw_wait(jlong millis, Thread* self);
   int raw_notify(Thread* self);
   int raw_notifyAll(Thread* self);
   int magic() const { return _magic; }

src/hotspot/share/runtime/objectMonitor.cpp

@@ -1267,6 +1267,10 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) {
 
   int ret = OS_OK;
   int WasNotified = 0;
+
+  // Need to check interrupt state whilst still _thread_in_vm
+  bool interrupted = interruptible && jt->is_interrupted(false);
+
   { // State transition wrappers
     OSThread* osthread = Self->osthread();
     OSThreadWaitState osts(osthread, true);
@@ -1275,7 +1279,7 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) {
       // Thread is in thread_blocked state and oop access is unsafe.
       jt->set_suspend_equivalent();
 
-      if (interruptible && (jt->is_interrupted(false) || HAS_PENDING_EXCEPTION)) {
+      if (interrupted || HAS_PENDING_EXCEPTION) {
         // Intentionally empty
       } else if (node._notified == 0) {
         if (millis <= 0) {

test/hotspot/jtreg/ProblemList.txt

@@ -206,16 +206,4 @@ vmTestbase/vm/mlvm/indy/func/jvmti/mergeCP_indy2manySame_b/TestDescription.java
 
 vmTestbase/nsk/jdwp/ThreadReference/ForceEarlyReturn/forceEarlyReturn001/forceEarlyReturn001.java 7199837 generic-all
 
-vmTestbase/nsk/jvmti/scenarios/allocation/AP01/ap01t001/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t001/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t002/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/allocation/AP04/ap04t003/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/allocation/AP10/ap10t001/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/allocation/AP12/ap12t001/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/capability/CM02/cm02t001/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t002/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t003/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t005/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/events/EM02/em02t006/TestDescription.java 8233549 generic-all
-vmTestbase/nsk/jvmti/scenarios/events/EM07/em07t002/TestDescription.java 8233549 generic-all
 #############################################################################