8264393: JDK-8258284 introduced dangling TLH race

src/hotspot/share/runtime/thread.hpp

@@ -173,11 +173,12 @@ class Thread: public ThreadShadow {
   friend class ScanHazardPtrPrintMatchingThreadsClosure;  // for get_threads_hazard_ptr(), is_hazard_ptr_tagged() access
   friend class ThreadsSMRSupport;  // for _nested_threads_hazard_ptr_cnt, _threads_hazard_ptr, _threads_list_ptr access
   friend class ThreadsListHandleTest;  // for _nested_threads_hazard_ptr_cnt, _threads_hazard_ptr, _threads_list_ptr access
+  friend class ValidateHazardPtrsClosure;  // for get_threads_hazard_ptr(), untag_hazard_ptr() access
 
   ThreadsList* volatile _threads_hazard_ptr;
   SafeThreadsListPtr*   _threads_list_ptr;
   ThreadsList*          cmpxchg_threads_hazard_ptr(ThreadsList* exchange_value, ThreadsList* compare_value);
-  ThreadsList*          get_threads_hazard_ptr();
+  ThreadsList*          get_threads_hazard_ptr() const;
   void                  set_threads_hazard_ptr(ThreadsList* new_list);
   static bool           is_hazard_ptr_tagged(ThreadsList* list) {
     return (intptr_t(list) & intptr_t(1)) == intptr_t(1);

src/hotspot/share/runtime/thread.inline.hpp

@@ -87,7 +87,7 @@ inline ThreadsList* Thread::cmpxchg_threads_hazard_ptr(ThreadsList* exchange_val
   return (ThreadsList*)Atomic::cmpxchg(&_threads_hazard_ptr, compare_value, exchange_value);
 }
 
-inline ThreadsList* Thread::get_threads_hazard_ptr() {
+inline ThreadsList* Thread::get_threads_hazard_ptr() const {
   return (ThreadsList*)Atomic::load_acquire(&_threads_hazard_ptr);
 }
 

src/hotspot/share/runtime/threadSMR.cpp

@@ -282,6 +282,9 @@ class ScanHazardPtrGatherProtectedThreadsClosure : public ThreadClosure {
       if (thread->cmpxchg_threads_hazard_ptr(NULL, current_list) == current_list) return;
     }
 
+    guarantee(ThreadsList::is_valid(current_list), "current_list="
+              INTPTR_FORMAT " is not valid!", p2i(current_list));
+
     // The current JavaThread has a hazard ptr (ThreadsList reference)
     // which might be _java_thread_list or it might be an older
     // ThreadsList that has been removed but not freed. In either case,
@@ -294,6 +297,10 @@ class ScanHazardPtrGatherProtectedThreadsClosure : public ThreadClosure {
 
 // Closure to gather hazard ptrs (ThreadsList references) into a hash table.
 //
+// Since this closure gathers hazard ptrs that may be tagged, this hash
+// table of hazard ptrs should only be used for value comparison and not
+// traversal of the ThreadsList.
+//
 class ScanHazardPtrGatherThreadsListClosure : public ThreadClosure {
  private:
   ThreadScanHashtable *_table;
@@ -304,18 +311,23 @@ class ScanHazardPtrGatherThreadsListClosure : public ThreadClosure {
     assert_locked_or_safepoint(Threads_lock);
 
     if (thread == NULL) return;
-    ThreadsList *threads = thread->get_threads_hazard_ptr();
-    if (threads == NULL) {
-      return;
+    ThreadsList *hazard_ptr = thread->get_threads_hazard_ptr();
+    if (hazard_ptr == NULL) return;
+    if (!Thread::is_hazard_ptr_tagged(hazard_ptr)) {
+      // We only validate hazard_ptrs that are not tagged since a tagged
+      // hazard ptr can be deleted at any time.
+      guarantee(ThreadsList::is_valid(hazard_ptr), "hazard_ptr=" INTPTR_FORMAT
+                " for thread=" INTPTR_FORMAT " is not valid!", p2i(hazard_ptr),
+                p2i(thread));
     }
     // In this closure we always ignore the tag that might mark this
     // hazard ptr as not yet verified. If we happen to catch an
     // unverified hazard ptr that is subsequently discarded (not
     // published), then the only side effect is that we might keep a
     // to-be-deleted ThreadsList alive a little longer.
-    threads = Thread::untag_hazard_ptr(threads);
-    if (!_table->has_entry((void*)threads)) {
-      _table->add_entry((void*)threads);
+    hazard_ptr = Thread::untag_hazard_ptr(hazard_ptr);
+    if (!_table->has_entry((void*)hazard_ptr)) {
+      _table->add_entry((void*)hazard_ptr);
     }
   }
 };
@@ -355,6 +367,27 @@ class ScanHazardPtrPrintMatchingThreadsClosure : public ThreadClosure {
   }
 };
 
+// Closure to validate hazard ptrs.
+//
+class ValidateHazardPtrsClosure : public ThreadClosure {
+ public:
+  ValidateHazardPtrsClosure() {};
+
+  virtual void do_thread(Thread* thread) {
+    assert_locked_or_safepoint(Threads_lock);
+
+    if (thread == NULL) return;
+    ThreadsList *hazard_ptr = thread->get_threads_hazard_ptr();
+    if (hazard_ptr == NULL) return;
+    // If the hazard ptr is unverified, then ignore it since it could
+    // be deleted at any time now.
+    if (Thread::is_hazard_ptr_tagged(hazard_ptr)) return;
+    guarantee(ThreadsList::is_valid(hazard_ptr), "hazard_ptr=" INTPTR_FORMAT
+              " for thread=" INTPTR_FORMAT " is not valid!", p2i(hazard_ptr),
+              p2i(thread));
+  }
+};
+
 // Closure to determine if the specified JavaThread is found by
 // threads_do().
 //
@@ -384,7 +417,7 @@ void SafeThreadsListPtr::acquire_stable_list() {
   _previous = _thread->_threads_list_ptr;
   _thread->_threads_list_ptr = this;
 
-  if (_thread->get_threads_hazard_ptr() == NULL) {
+  if (_thread->get_threads_hazard_ptr() == NULL && _previous == NULL) {
     // The typical case is first.
     acquire_stable_list_fast_path();
     return;
@@ -403,7 +436,7 @@ void SafeThreadsListPtr::acquire_stable_list_fast_path() {
   ThreadsList* threads;
 
   // Stable recording of a hazard ptr for SMR. This code does not use
-  // locks so its use of the _smr_java_thread_list & _threads_hazard_ptr
+  // locks so its use of the _java_thread_list & _threads_hazard_ptr
   // fields is racy relative to code that uses those fields with locks.
   // OrderAccess and Atomic functions are used to deal with those races.
   //
@@ -419,7 +452,7 @@ void SafeThreadsListPtr::acquire_stable_list_fast_path() {
     ThreadsList* unverified_threads = Thread::tag_hazard_ptr(threads);
     _thread->set_threads_hazard_ptr(unverified_threads);
 
-    // If _smr_java_thread_list has changed, we have lost a race with
+    // If _java_thread_list has changed, we have lost a race with
     // Threads::add() or Threads::remove() and have to try again.
     if (ThreadsSMRSupport::get_java_thread_list() != threads) {
       continue;
@@ -451,8 +484,6 @@ void SafeThreadsListPtr::acquire_stable_list_fast_path() {
 //
 void SafeThreadsListPtr::acquire_stable_list_nested_path() {
   assert(_thread != NULL, "sanity check");
-  assert(_thread->get_threads_hazard_ptr() != NULL,
-         "cannot have a NULL regular hazard ptr when acquiring a nested hazard ptr");
 
   // The thread already has a hazard ptr (ThreadsList ref) so we need
   // to create a nested ThreadsListHandle with the current ThreadsList
@@ -473,7 +504,7 @@ void SafeThreadsListPtr::acquire_stable_list_nested_path() {
   }
   // Clear the hazard ptr so we can go through the fast path below and
   // acquire a nested stable ThreadsList.
-  Atomic::store(&_thread->_threads_hazard_ptr, (ThreadsList*)NULL);
+  _thread->set_threads_hazard_ptr(NULL);
 
   if (EnableThreadSMRStatistics && _thread->nested_threads_hazard_ptr_cnt() > ThreadsSMRSupport::_nested_thread_list_max) {
     ThreadsSMRSupport::_nested_thread_list_max = _thread->nested_threads_hazard_ptr_cnt();
@@ -490,11 +521,26 @@ void SafeThreadsListPtr::acquire_stable_list_nested_path() {
 //
 void SafeThreadsListPtr::release_stable_list() {
   assert(_thread != NULL, "sanity check");
-  assert(_thread->get_threads_hazard_ptr() != NULL, "sanity check");
-  assert(_thread->get_threads_hazard_ptr() == _list, "sanity check");
   assert(_thread->_threads_list_ptr == this, "sanity check");
   _thread->_threads_list_ptr = _previous;
 
+  // We're releasing either a leaf or nested ThreadsListHandle. In either
+  // case, we set this thread's hazard ptr back to NULL and we do it before
+  // _nested_handle_cnt is decremented below.
+  _thread->set_threads_hazard_ptr(NULL);
+  if (_previous != NULL) {
+    // The ThreadsListHandle being released is a nested ThreadsListHandle.
+    if (EnableThreadSMRStatistics) {
+      _thread->dec_nested_threads_hazard_ptr_cnt();
+    }
+    // The previous ThreadsList is stable because the _nested_handle_cnt is
+    // > 0, but we cannot safely make it this thread's hazard ptr again.
+    // The protocol for installing and verifying a ThreadsList as a
+    // thread's hazard ptr is handled by acquire_stable_list_fast_path().
+    // And that protocol cannot be properly done with a ThreadsList that
+    // might not be the current system ThreadsList.
+    assert(_previous->_list->_nested_handle_cnt > 0, "must be > zero");
+  }
   if (_has_ref_count) {
     // This thread created a nested ThreadsListHandle after the current
     // ThreadsListHandle so we had to protect this ThreadsList with a
@@ -503,23 +549,6 @@ void SafeThreadsListPtr::release_stable_list() {
 
     log_debug(thread, smr)("tid=" UINTX_FORMAT ": SafeThreadsListPtr::release_stable_list: delete nested list pointer to ThreadsList=" INTPTR_FORMAT, os::current_thread_id(), p2i(_list));
   }
-  if (_previous == NULL) {
-    // The ThreadsListHandle being released is a leaf ThreadsListHandle.
-    // This is the "normal" case and this is where we set this thread's
-    // hazard ptr back to NULL.
-    _thread->set_threads_hazard_ptr(NULL);
-  } else {
-    // The ThreadsListHandle being released is a nested ThreadsListHandle.
-    if (EnableThreadSMRStatistics) {
-      _thread->dec_nested_threads_hazard_ptr_cnt();
-    }
-    // The previous ThreadsList becomes this thread's hazard ptr again.
-    // It is a stable ThreadsList since the non-zero _nested_handle_cnt
-    // keeps it from being freed so we can just set the thread's hazard
-    // ptr without going through the stabilization/tagging protocol.
-    assert(_previous->_list->_nested_handle_cnt > 0, "must be > than zero");
-    _thread->set_threads_hazard_ptr(_previous->_list);
-  }
 
   // After releasing the hazard ptr, other threads may go ahead and
   // free up some memory temporarily used by a ThreadsList snapshot.
@@ -530,6 +559,10 @@ void SafeThreadsListPtr::release_stable_list() {
     // An exiting thread might be waiting in smr_delete(); we need to
     // check with delete_lock to be sure.
     ThreadsSMRSupport::release_stable_list_wake_up(_has_ref_count);
+    guarantee(_previous == NULL || ThreadsList::is_valid(_previous->_list),
+              "_previous->_list=" INTPTR_FORMAT
+              " is not valid after calling release_stable_list_wake_up!",
+              p2i(_previous->_list));
   }
 }
 
@@ -601,6 +634,7 @@ static JavaThread* const* make_threads_list_data(int entries) {
 }
 
 ThreadsList::ThreadsList(int entries) :
+  _magic(THREADS_LIST_MAGIC),
   _length(entries),
   _next_list(NULL),
   _threads(make_threads_list_data(entries)),
@@ -611,6 +645,7 @@ ThreadsList::~ThreadsList() {
   if (_threads != empty_threads_list_data) {
     FREE_C_HEAP_ARRAY(JavaThread*, _threads);
   }
+  _magic = 0xDEADBEEF;
 }
 
 // Add a JavaThread to a ThreadsList. The returned ThreadsList is a
@@ -882,6 +917,9 @@ void ThreadsSMRSupport::free_list(ThreadsList* threads) {
     log_debug(thread, smr)("tid=" UINTX_FORMAT ": ThreadsSMRSupport::free_list: threads=" INTPTR_FORMAT " is not freed.", os::current_thread_id(), p2i(threads));
   }
 
+  ValidateHazardPtrsClosure validate_cl;
+  threads_do(&validate_cl);
+
   delete scan_table;
 }
 
@@ -1096,8 +1134,9 @@ void ThreadsSMRSupport::log_statistics() {
 
 // Print SMR info for a thread to a given output stream.
 void ThreadsSMRSupport::print_info_on(const Thread* thread, outputStream* st) {
-  if (thread->_threads_hazard_ptr != NULL) {
-    st->print(" _threads_hazard_ptr=" INTPTR_FORMAT, p2i(thread->_threads_hazard_ptr));
+  ThreadsList* hazard_ptr = thread->get_threads_hazard_ptr();
+  if (hazard_ptr != NULL) {
+    st->print(" _threads_hazard_ptr=" INTPTR_FORMAT, p2i(hazard_ptr));
   }
   if (EnableThreadSMRStatistics && thread->_threads_list_ptr != NULL) {
     // The count is only interesting if we have a _threads_list_ptr.

src/hotspot/share/runtime/threadSMR.hpp

@@ -162,11 +162,13 @@ class ThreadsSMRSupport : AllStatic {
 // A fast list of JavaThreads.
 //
 class ThreadsList : public CHeapObj<mtThread> {
+  enum { THREADS_LIST_MAGIC = (int)(('T' << 24) | ('L' << 16) | ('S' << 8) | 'T') };
   friend class VMStructs;
   friend class SafeThreadsListPtr;  // for {dec,inc}_nested_handle_cnt() access
   friend class ThreadsSMRSupport;  // for _nested_handle_cnt, {add,remove}_thread(), {,set_}next_list() access
   friend class ThreadsListHandleTest;  // for _nested_handle_cnt access
 
+  uint _magic;
   const uint _length;
   ThreadsList* _next_list;
   JavaThread *const *const _threads;
@@ -203,6 +205,8 @@ class ThreadsList : public CHeapObj<mtThread> {
   int find_index_of_JavaThread(JavaThread* target);
   JavaThread* find_JavaThread_from_java_tid(jlong java_tid) const;
   bool includes(const JavaThread * const p) const;
+
+  static bool is_valid(ThreadsList* list) { return list->_magic == THREADS_LIST_MAGIC; }
 };
 
 // An abstract safe ptr to a ThreadsList comprising either a stable hazard ptr

test/hotspot/gtest/runtime/test_ThreadsListHandle.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -213,9 +213,10 @@ TEST_VM(ThreadsListHandle, sanity) {
 
     // Test case: after first nested ThreadsListHandle (tlh2) has been destroyed
 
-    // Verify the current thread refers to tlh1:
-    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), tlh1.list())
-        << "thr->_threads_hazard_ptr must match tlh1.list()";
+    // Verify the current thread's hazard ptr is NULL:
+    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), (ThreadsList*)NULL)
+        << "thr->_threads_hazard_ptr must be NULL";
+    // Verify the current thread's threads list ptr refers to tlh1:
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_list_ptr(thr), list_ptr1)
         << "thr->_threads_list_ptr must match list_ptr1";
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_nested_threads_hazard_ptr_cnt(thr), (uint)0)
@@ -396,9 +397,10 @@ TEST_VM(ThreadsListHandle, sanity) {
 
       // Test case: after double nested ThreadsListHandle (tlh3) has been destroyed
 
-      // Verify the current thread refers to tlh2:
-      EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), tlh2.list())
-          << "thr->_threads_hazard_ptr must match tlh2.list()";
+      // Verify the current thread's hazard ptr is NULL:
+      EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), (ThreadsList*)NULL)
+          << "thr->_threads_hazard_ptr must be NULL";
+      // Verify the current thread's threads list ptr refers to tlh2:
       EXPECT_EQ(tlh1.list(), tlh2.list())
           << "tlh1.list() must match tlh2.list()";
       EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_list_ptr(thr), list_ptr2)
@@ -443,9 +445,10 @@ TEST_VM(ThreadsListHandle, sanity) {
 
     // Test case: after first nested ThreadsListHandle (tlh2) has been destroyed
 
-    // Verify the current thread refers to tlh1:
-    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), tlh1.list())
-        << "thr->_threads_hazard_ptr must match tlh1.list()";
+    // Verify the current thread's hazard ptr is NULL:
+    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), (ThreadsList*)NULL)
+        << "thr->_threads_hazard_ptr must be NULL";
+    // Verify the current thread's threads list ptr refers to tlh1:
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_list_ptr(thr), list_ptr1)
         << "thr->_threads_list_ptr must match list_ptr1";
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_nested_threads_hazard_ptr_cnt(thr), (uint)0)
@@ -562,9 +565,10 @@ TEST_VM(ThreadsListHandle, sanity) {
 
     // Test case: after first back-to-back nested ThreadsListHandle (tlh2a) has been destroyed
 
-    // Verify the current thread refers to tlh1:
-    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), tlh1.list())
-        << "thr->_threads_hazard_ptr must match tlh1.list()";
+    // Verify the current thread's hazard ptr is NULL:
+    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), (ThreadsList*)NULL)
+        << "thr->_threads_hazard_ptr must be NULL";
+    // Verify the current thread's threads list ptr refers to tlh1:
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_list_ptr(thr), list_ptr1)
         << "thr->_threads_list_ptr must match list_ptr1";
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_nested_threads_hazard_ptr_cnt(thr), (uint)0)
@@ -639,9 +643,10 @@ TEST_VM(ThreadsListHandle, sanity) {
 
     // Test case: after second back-to-back nested ThreadsListHandle (tlh2b) has been destroyed
 
-    // Verify the current thread refers to tlh1:
-    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), tlh1.list())
-        << "thr->_threads_hazard_ptr must match tlh1.list()";
+    // Verify the current thread's hazard ptr is NULL:
+    EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_hazard_ptr(thr), (ThreadsList*)NULL)
+        << "thr->_threads_hazard_ptr must be NULL";
+    // Verify the current thread's threads list ptr refers to tlh1:
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_threads_list_ptr(thr), list_ptr1)
         << "thr->_threads_list_ptr must match list_ptr1";
     EXPECT_EQ(ThreadsListHandleTest::get_Thread_nested_threads_hazard_ptr_cnt(thr), (uint)0)

test/hotspot/jtreg/runtime/ErrorHandling/NestedThreadsListHandleInErrorHandlingTest.java

@@ -98,8 +98,8 @@ public static void main(String[] args) throws Exception {
         // a ThreadsListHandle in addition to what the test creates:
         Pattern.compile(".*, _nested_thread_list_max=2"),
         // The current thread (marked with '=>') in the threads list
-        // should show a hazard ptr and a nested hazard ptr:
-        Pattern.compile("=>.* JavaThread \"main\" .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=1, _nested_threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*.*"),
+        // should show a nested hazard ptr:
+        Pattern.compile("=>.* JavaThread \"main\" .*, _nested_threads_hazard_ptr_cnt=1, _nested_threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*.*"),
     };
     int currentPattern = 0;
 

test/hotspot/jtreg/runtime/ErrorHandling/ThreadsListHandleInErrorHandlingTest.java

@@ -98,8 +98,8 @@ public static void main(String[] args) throws Exception {
         // a ThreadsListHandle:
         Pattern.compile(".*, _nested_thread_list_max=1"),
         // The current thread (marked with '=>') in the threads list
-        // should show a hazard ptr and no nested hazard ptrs:
-        Pattern.compile("=>.* JavaThread \"main\" .* _threads_hazard_ptr=0x[0-9A-Fa-f][0-9A-Fa-f]*, _nested_threads_hazard_ptr_cnt=0.*"),
+        // should show no nested hazard ptrs:
+        Pattern.compile("=>.* JavaThread \"main\" .*, _nested_threads_hazard_ptr_cnt=0.*"),
     };
     int currentPattern = 0;