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

[David Holmes]

On Mon, 24 Nov 2025 18:40:22 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:
> 
>   Use put_when_absent and add an assert that the message is the same.

Fix looks good, but an additional check requested. Thanks

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, and set it again.
> 1877:       assert(entry->nest_host_error() == nullptr || strcmp(entry->nest_host_error(), message) == 0, "should be the same message");

Rather than overwrite the message I would keep the existing null check and maintain three cases:
1. No entry so add new one with message
2. Existing entry with no-message, so add it
3. Existing entry with message - just assert message is the same.

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

> 1876:       assert(pool->resolved_klass_at(which) != nullptr, "klass is should be resolved if there is no entry");
> 1877:       ResolutionErrorTable::add_entry(pool, which, message);
> 1878:     }

Suggestion:

    } else if (entry == nullptr) {
      // Only add a new resolution error if one hasn't been found for this constant pool index. In this case,
      // resolution succeeded but there's an error in this nest host.
      assert(pool->resolved_klass_at(which) != nullptr, "klass is should be resolved if there is no entry");
      ResolutionErrorTable::add_entry(pool, which, message);
    } else {
      assert(strcmp(entry->nest_host_error(), message) == 0, "Different nest host error messages found: existing: %s, new %s", entry->nest_host_error(), message);
    }

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/28438#pullrequestreview-3498002822
PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2558214603
PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2554325963