Skip to content

Commit

Permalink
8313678: SymbolTable can leak Symbols during cleanup
Browse files Browse the repository at this point in the history
Reviewed-by: phh, shade
Backport-of: 4b2703ad39f8160264eb30c797824cc93a6b56e2
  • Loading branch information
olivergillespie authored and Paul Hohensee committed Aug 29, 2023
1 parent c91fb3f commit a75e5de
Show file tree
Hide file tree
Showing 7 changed files with 54 additions and 23 deletions.
17 changes: 11 additions & 6 deletions src/hotspot/share/classfile/stringTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 {
Expand All @@ -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);
Expand All @@ -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) {
Expand Down
11 changes: 8 additions & 3 deletions src/hotspot/share/classfile/symbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down
8 changes: 5 additions & 3 deletions src/hotspot/share/prims/resolvedMethodTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}
};


Expand Down
5 changes: 4 additions & 1 deletion src/hotspot/share/services/threadIdTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
12 changes: 4 additions & 8 deletions src/hotspot/share/utilities/concurrentHashTable.inline.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
14 changes: 14 additions & 0 deletions test/hotspot/gtest/classfile/test_symbolTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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";
}
10 changes: 8 additions & 2 deletions test/hotspot/gtest/utilities/test_concurrentHashtable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand Down

1 comment on commit a75e5de

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.