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

[Johan Sjölen]

On Sat, 22 Nov 2025 00:30:40 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> 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/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& pool, int which,
                                            Symbol* error, Borrow<const char> message,
                                            Symbol* cause, Borrow<const char> cause_msg) : 
  // Reader meant to think: Wait, we're assigning a Borrow to an Own directly? Seems wrong.
    _message(message),
// Reader meant to think: Aah, we're making a copy to get ownership
   _cause_msg(os::strdup(cause_msg)) 
{
  /* ... */
}
};


This will make no compiler errors for us in case of incorrect usage, but it will be a sign to the reader that `SystemDictionary` doesn't intend to clean up `message` or `cause_msg`, and that the writer actually thought about the possibility of a leak from these strings.

**I'm not suggesting this is what we add,** I'm just saying that clearly we can communicate more in the code than we currently do.

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

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