8313678: SymbolTable can leak Symbols during cleanup
Reviewed-by: coleenp, dholmes, shade
This commit is contained in:
parent
f41c267f85
commit
4b2703ad39
src/hotspot/share
classfile
gc/g1
prims
services
utilities
test/hotspot
gtest
jtreg/runtime/cds/appcds/dynamicArchive
@ -229,11 +229,13 @@ public:
|
||||
uintx get_hash() const {
|
||||
return _name->identity_hash();
|
||||
}
|
||||
bool equals(DictionaryEntry** value, bool* is_dead) {
|
||||
bool equals(DictionaryEntry** value) {
|
||||
DictionaryEntry *entry = *value;
|
||||
*is_dead = false;
|
||||
return (entry->instance_klass()->name() == _name);
|
||||
}
|
||||
bool is_dead(DictionaryEntry** value) {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
|
||||
// Add a loaded class to the dictionary.
|
||||
|
@ -176,11 +176,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 == nullptr) {
|
||||
// dead oop, mark this hash dead for cleaning
|
||||
*is_dead = true;
|
||||
return false;
|
||||
}
|
||||
bool equals = java_lang_String::equals(val_oop, _str, _len);
|
||||
@ -191,6 +189,10 @@ class StringTableLookupJchar : StackObj {
|
||||
_found = Handle(_thread, value->resolve());
|
||||
return true;
|
||||
}
|
||||
bool is_dead(WeakHandle* value) {
|
||||
oop val_oop = value->peek();
|
||||
return val_oop == nullptr;
|
||||
}
|
||||
};
|
||||
|
||||
class StringTableLookupOop : public StackObj {
|
||||
@ -208,11 +210,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 == nullptr) {
|
||||
// dead oop, mark this hash dead for cleaning
|
||||
*is_dead = true;
|
||||
return false;
|
||||
}
|
||||
bool equals = java_lang_String::equals(_find(), val_oop);
|
||||
@ -223,6 +223,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 == nullptr;
|
||||
}
|
||||
};
|
||||
|
||||
void StringTable::create_table() {
|
||||
|
@ -374,7 +374,11 @@ public:
|
||||
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 != nullptr, "expected valid value");
|
||||
Symbol *sym = value;
|
||||
if (sym->equals(_str, _len)) {
|
||||
@ -383,14 +387,15 @@ public:
|
||||
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 {
|
||||
|
@ -258,10 +258,13 @@ class G1CardSetHashTable : public CHeapObj<mtGCCardSet> {
|
||||
|
||||
uintx get_hash() const { return G1CardSetHashTable::get_hash(_region_idx); }
|
||||
|
||||
bool equals(G1CardSetHashTableValue* value, bool* is_dead) {
|
||||
*is_dead = false;
|
||||
bool equals(G1CardSetHashTableValue* value) {
|
||||
return value->_region_idx == _region_idx;
|
||||
}
|
||||
|
||||
bool is_dead(G1CardSetHashTableValue*) {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
|
||||
class G1CardSetHashTableFound : public StackObj {
|
||||
|
@ -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 == nullptr) {
|
||||
// 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 == nullptr;
|
||||
}
|
||||
};
|
||||
|
||||
|
||||
|
@ -137,11 +137,14 @@ class FinalizerEntryLookup : StackObj {
|
||||
public:
|
||||
FinalizerEntryLookup(const InstanceKlass* ik) : _ik(ik) {}
|
||||
uintx get_hash() const { return hash_function(_ik); }
|
||||
bool equals(FinalizerEntry** value, bool* is_dead) {
|
||||
bool equals(FinalizerEntry** value) {
|
||||
assert(value != nullptr, "invariant");
|
||||
assert(*value != nullptr, "invariant");
|
||||
return (*value)->klass() == _ik;
|
||||
}
|
||||
bool is_dead(FinalizerEntry** value) {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
|
||||
class FinalizerTableConfig : public AllStatic {
|
||||
|
@ -187,13 +187,16 @@ public:
|
||||
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 {
|
||||
|
@ -455,9 +455,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 != nullptr) {
|
||||
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 {
|
||||
@ -546,9 +545,7 @@ inline void ConcurrentHashTable<CONFIG, F>::
|
||||
Node* const volatile * rem_n_prev = bucket->first_ptr();
|
||||
Node* rem_n = bucket->first();
|
||||
while (rem_n != nullptr) {
|
||||
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);
|
||||
@ -626,12 +623,11 @@ ConcurrentHashTable<CONFIG, F>::
|
||||
size_t loop_count = 0;
|
||||
Node* node = bucket->first();
|
||||
while (node != nullptr) {
|
||||
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();
|
||||
|
@ -125,3 +125,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";
|
||||
}
|
||||
|
@ -107,9 +107,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 {
|
||||
@ -561,9 +564,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) {
|
||||
|
@ -90,7 +90,7 @@ public class DynamicSharedSymbols extends DynamicArchiveTestBase {
|
||||
ProcessBuilder pb = new ProcessBuilder();
|
||||
pb.command(new String[] {JDKToolFinder.getJDKTool("jcmd"), Long.toString(pid), "VM.symboltable", "-verbose"});
|
||||
OutputAnalyzer output = CDSTestUtils.executeAndLog(pb, "jcmd-symboltable");
|
||||
output.shouldContain("17 3: jdk/test/lib/apps\n");
|
||||
output.shouldContain("17 2: jdk/test/lib/apps\n");
|
||||
output.shouldContain("Dynamic shared symbols:\n");
|
||||
output.shouldContain("5 65535: Hello\n");
|
||||
|
||||
|
Loading…
x
Reference in New Issue
Block a user