8313678: SymbolTable can leak Symbols during cleanup

Reviewed-by: phh, shade
Backport-of: 4b2703ad39f8160264eb30c797824cc93a6b56e2

src/hotspot/share/classfile/stringTable.cpp

@@ -151,11 +151,9 @@ class StringTableLookupJchar : StackObj {
   uintx get_hash() const {
     return _hash;
   }
-  bool equals(WeakHandle* value, bool* is_dead) {
+  bool equals(WeakHandle* value) {
     oop val_oop = value->peek();
     if (val_oop == NULL) {
-      // dead oop, mark this hash dead for cleaning
-      *is_dead = true;
       return false;
     }
     bool equals = java_lang_String::equals(val_oop, _str, _len);
@@ -166,6 +164,10 @@ class StringTableLookupJchar : StackObj {
      _found = Handle(_thread, value->resolve());
     return true;
   }
+  bool is_dead(WeakHandle* value) {
+    oop val_oop = value->peek();
+    return val_oop == NULL;
+  }
 };
 
 class StringTableLookupOop : public StackObj {
@@ -183,11 +185,9 @@ class StringTableLookupOop : public StackObj {
     return _hash;
   }
 
-  bool equals(WeakHandle* value, bool* is_dead) {
+  bool equals(WeakHandle* value) {
     oop val_oop = value->peek();
     if (val_oop == NULL) {
-      // dead oop, mark this hash dead for cleaning
-      *is_dead = true;
       return false;
     }
     bool equals = java_lang_String::equals(_find(), val_oop);
@@ -198,6 +198,11 @@ class StringTableLookupOop : public StackObj {
     _found = Handle(_thread, value->resolve());
     return true;
   }
+
+  bool is_dead(WeakHandle* value) {
+    oop val_oop = value->peek();
+    return val_oop == NULL;
+  }
 };
 
 static size_t ceil_log2(size_t val) {

src/hotspot/share/classfile/symbolTable.cpp

@@ -373,7 +373,11 @@ class SymbolTableLookup : StackObj {
   uintx get_hash() const {
     return _hash;
   }
-  bool equals(Symbol** value, bool* is_dead) {
+  // Note: When equals() returns "true", the symbol's refcount is incremented. This is
+  // needed to ensure that the symbol is kept alive before equals() returns to the caller,
+  // so that another thread cannot clean the symbol up concurrently. The caller is
+  // responsible for decrementing the refcount, when the symbol is no longer needed.
+  bool equals(Symbol** value) {
     assert(value != NULL, "expected valid value");
     assert(*value != NULL, "value should point to a symbol");
     Symbol *sym = *value;
@@ -383,14 +387,15 @@ class SymbolTableLookup : StackObj {
         return true;
       } else {
         assert(sym->refcount() == 0, "expected dead symbol");
-        *is_dead = true;
         return false;
       }
     } else {
-      *is_dead = (sym->refcount() == 0);
       return false;
     }
   }
+  bool is_dead(Symbol** value) {
+    return (*value)->refcount() == 0;
+  }
 };
 
 class SymbolTableGet : public StackObj {

src/hotspot/share/prims/resolvedMethodTable.cpp

@@ -126,11 +126,9 @@ class ResolvedMethodTableLookup : StackObj {
   uintx get_hash() const {
     return _hash;
   }
-  bool equals(WeakHandle* value, bool* is_dead) {
+  bool equals(WeakHandle* value) {
     oop val_oop = value->peek();
     if (val_oop == NULL) {
-      // dead oop, mark this hash dead for cleaning
-      *is_dead = true;
       return false;
     }
     bool equals = _method == java_lang_invoke_ResolvedMethodName::vmtarget(val_oop);
@@ -141,6 +139,10 @@ class ResolvedMethodTableLookup : StackObj {
     _found = Handle(_thread, value->resolve());
     return true;
   }
+  bool is_dead(WeakHandle* value) {
+    oop val_oop = value->peek();
+    return val_oop == NULL;
+  }
 };
 
 

src/hotspot/share/services/threadIdTable.cpp

@@ -192,13 +192,16 @@ class ThreadIdTableLookup : public StackObj {
   uintx get_hash() const {
     return _hash;
   }
-  bool equals(ThreadIdTableEntry** value, bool* is_dead) {
+  bool equals(ThreadIdTableEntry** value) {
     bool equals = primitive_equals(_tid, (*value)->tid());
     if (!equals) {
       return false;
     }
     return true;
   }
+  bool is_dead(ThreadIdTableEntry** value) {
+    return false;
+  }
 };
 
 class ThreadGet : public StackObj {

src/hotspot/share/utilities/concurrentHashTable.inline.hpp

@@ -451,9 +451,8 @@ inline bool ConcurrentHashTable<CONFIG, F>::
   assert(bucket->is_locked(), "Must be locked.");
   Node* const volatile * rem_n_prev = bucket->first_ptr();
   Node* rem_n = bucket->first();
-  bool have_dead = false;
   while (rem_n != NULL) {
-    if (lookup_f.equals(rem_n->value(), &have_dead)) {
+    if (lookup_f.equals(rem_n->value())) {
       bucket->release_assign_node_ptr(rem_n_prev, rem_n->next());
       break;
     } else {
@@ -540,9 +539,7 @@ inline void ConcurrentHashTable<CONFIG, F>::
   Node* const volatile * rem_n_prev = bucket->first_ptr();
   Node* rem_n = bucket->first();
   while (rem_n != NULL) {
-    bool is_dead = false;
-    lookup_f.equals(rem_n->value(), &is_dead);
-    if (is_dead) {
+    if (lookup_f.is_dead(rem_n->value())) {
       ndel[dels++] = rem_n;
       Node* next_node = rem_n->next();
       bucket->release_assign_node_ptr(rem_n_prev, next_node);
@@ -620,12 +617,11 @@ ConcurrentHashTable<CONFIG, F>::
   size_t loop_count = 0;
   Node* node = bucket->first();
   while (node != NULL) {
-    bool is_dead = false;
     ++loop_count;
-    if (lookup_f.equals(node->value(), &is_dead)) {
+    if (lookup_f.equals(node->value())) {
       break;
     }
-    if (is_dead && !(*have_dead)) {
+    if (!(*have_dead) && lookup_f.is_dead(node->value())) {
       *have_dead = true;
     }
     node = node->next();

test/hotspot/gtest/classfile/test_symbolTable.cpp

@@ -152,3 +152,17 @@ TEST_VM_FATAL_ERROR_MSG(SymbolTable, test_symbol_underflow, ".*refcount has gone
   my_symbol->decrement_refcount();
   my_symbol->increment_refcount();  // Should crash even in PRODUCT mode
 }
+
+TEST_VM(SymbolTable, test_cleanup_leak) {
+  // Check that dead entry cleanup doesn't increment refcount of live entry in same bucket.
+
+  // Create symbol and release ref, marking it available for cleanup.
+  Symbol* entry1 = SymbolTable::new_symbol("hash_collision_123");
+  entry1->decrement_refcount();
+
+  // Create a new symbol in the same bucket, which will notice the dead entry and trigger cleanup.
+  // Note: relies on SymbolTable's use of String::hashCode which collides for these two values.
+  Symbol* entry2 = SymbolTable::new_symbol("hash_collision_397476851");
+
+  ASSERT_EQ(entry2->refcount(), 1) << "Symbol refcount just created is 1";
+}

test/hotspot/gtest/utilities/test_concurrentHashtable.cpp

@@ -105,9 +105,12 @@ struct SimpleTestLookup {
   uintx get_hash() {
     return Pointer::get_hash(_val, NULL);
   }
-  bool equals(const uintptr_t* value, bool* is_dead) {
+  bool equals(const uintptr_t* value) {
     return _val == *value;
   }
+  bool is_dead(const uintptr_t* value) {
+    return false;
+  }
 };
 
 struct ValueGet {
@@ -533,9 +536,12 @@ struct TestLookup {
   uintx get_hash() {
     return TestInterface::get_hash(_val, NULL);
   }
-  bool equals(const uintptr_t* value, bool* is_dead) {
+  bool equals(const uintptr_t* value) {
     return _val == *value;
   }
+  bool is_dead(const uintptr_t* value) {
+    return false;
+  }
 };
 
 static uintptr_t cht_get_copy(TestTable* cht, Thread* thr, TestLookup tl) {