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

[Johan Sjölen]

On Sat, 22 Nov 2025 10:12:09 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> 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 <<<<<<<<<<
>>     }
>>   }
>> }
>
> In the `SystemDictionary` case, we're fine. You wouldn't think so, but we are. That's because message and cause_msg are resource allocated, and those strings are strdup:ed in the constructor of the table entry. `InstanceKlass::next_host` has a memory leak though, because `ResolutionErrorEntry` *does* take ownership of the underlying string pointer, so we have this:
> 
> ```c++
>       const char* msg = ss.as_string(true /* on C-heap */);
>       constantPoolHandle cph(THREAD, constants());
>       SystemDictionary::add_nest_host_error(cph, _nest_host_index, msg);
>       // ... down the callstack we go, reaching the constructor call:
>        ResolutionErrorEntry *entry = new ResolutionErrorEntry(message);
>        ResolutionErrorEntry(const char* message):
>         _error(nullptr),
>         _message(nullptr),
>         _cause(nullptr),
>         _cause_msg(nullptr),
> 
>         _nest_host_error(message) {} // <-- Noooo
> 
> 
> As opposed to the other constructor, which looks like this:
> 
> ```c++
> // This is the call to the constructor this time:
>   ResolutionErrorEntry *entry = new ResolutionErrorEntry(error, message, cause, cause_msg);
> ResolutionErrorEntry::ResolutionErrorEntry(Symbol* error, const char* message,
>                                            Symbol* cause, const char* cause_msg):
>         _error(error),
>         _message(message != nullptr ? os::strdup(message) : nullptr),
>         _cause(cause),
>         _cause_msg(cause_msg != nullptr ? os::strdup(cause_msg) : nullptr),
>         _nest_host_error(nullptr) {
> 
>   Symbol::maybe_increment_refcount(_error);
>   Symbol::maybe_increment_refcount(_cause);
> }
> 
> 
> This is actually pretty bad :-/, I'd really appreciate it if we could make these types of bugs a bit more shallow at the time of writing them.
> 
> Maybe it'd be nice to have a type that tells the reader that an object doesn't intend to free a received pointer on its destruction? This is a very small sketch of something illustrating kind of what I mean:
> 
> ```c++
> template<typename T>
> using Borrow = T*;
> template<typename T>
> using Own = T*;
> 
> // "I'll take a string, but I don't intend to be responsible for freeing it"
> const char* os::strdup(Borrow<const char>, MemTag) { /* ... */}
> 
> class SystemDictionary {
>   Own<const char> _message; // I own this, and so I intend to free it when I'm destroyed
>   Own<const char> _cause_msg; // Same here
> 
>   // "I'll take a message and a cause_msg, and I won't be responsible for freeing it"
>   void SystemDictionary::add_resolution_error(const constantPoolHandle& p...

I made a ticket for this: [8372373](https://bugs.openjdk.org/browse/JDK-8372373)

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28438#discussion_r2553100145