RFR: 8365526: Crash with null Symbol passed to SystemDictionary::resolve_or_null [v2]

[Ioi Lam]

On Fri, 21 Nov 2025 18:57:28 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> The vm was crashing because the constant pool couldn't find the resolution error in the ResolutionErrorEntry error field.
>> 
>> There are two uses of ResolutionErrorEntry in the ResolutionErrorTable.  The key to this table is {ConstantPool, cp-index}.   In this crash, multiple threads were racing to record nest_host_errors in the case where resolution failed.  In this case, there was already a ResolutionErrorEntry in the table for the constant pool resolution failure.  In the 'if' case of add_nest_host_error we check to see if there's already a nest_host_error assuming it's the same error, then the 'else' case was unconditionally adding a ResolutionErrorEntry with just the nest host message.  Calling HashTable::put() with this entry with just the nest host message, was overwriting the entry with the constant pool resolution error, ie. the other fields.  The crash happened in ConstantPool::throw_resolution_error() because the error field was overwritten (and leaked too).
>> 
>> Add a null check before calling ResolutionErrorEntry add entry.  Also added an assert that we only add a resolution error for nest host errors in the case of success since in the case of failure there will always already be a ResolutionErrorEntry for the failing constant pool and cp index and we don't want to overwrite that again.
>> 
>> Tested with submitted reproducer and tier1-4.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reorder if statement and add an assert.

src/hotspot/share/classfile/resolutionErrors.cpp line 90:

> 88:   bool created = false;
> 89:   _resolution_error_table->put_if_absent(key, entry, &created);
> 90:   assert(created, "should be created not updated");

The above 3 lines can be replaced with `_resolution_error_table->put_when_absent(key, entry)`.

src/hotspot/share/classfile/systemDictionary.cpp line 1870:

> 1868:       // Only add a new resolution error if one hasn't been found for this constant pool index. In this case,
> 1869:       // resolution succeeded but there's an error in this nest host.
> 1870:       assert(pool->resolved_klass_at(which) != nullptr, "klass is should be resolved if there is no entry");

typo: `"klass should be resolved ...`

src/hotspot/share/classfile/systemDictionary.cpp line 1877:

> 1875:       // only reach here under the same error condition, so we can ignore the potential race with setting
> 1876:       // the message. If we see it is already set then we can ignore it.
> 1877:       entry->set_nest_host_error(message);

Existing -- shouldn't we free the old `entry->_nest_host_error`?

Also, there's a related memory leak here:


// Add entry to resolution error table to record the error when the first
// attempt to resolve a reference to a class has failed.
void SystemDictionary::add_resolution_error(const constantPoolHandle& pool, int which,
                                            Symbol* error, const char* message,
                                            Symbol* cause, const char* cause_msg) {
  {
    MutexLocker ml(Thread::current(), SystemDictionary_lock);
    ResolutionErrorEntry* entry = ResolutionErrorTable::find_entry(pool, which);
    if (entry == nullptr) {
      ResolutionErrorTable::add_entry(pool, which, error, message, cause, cause_msg);
    } else {
        // message and cause_msg are leaked <<<<<<<<<<
    }
  }
}

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2551404434
PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2551510022
PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2551420213