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

[Coleen Phillimore]

On Sat, 22 Nov 2025 11:58:49 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> 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'...
>
> I made a ticket for this: [8372373](https://bugs.openjdk.org/browse/JDK-8372373)

As Johan said in the first case, the message and cause are resource allocated so not owned by the ResolvedErrorEntry, so that's not a leak.  I have to read the code around the nest host error again to comment.  I would not like this strange template around ownership.

Also, set_nest_host_error() does not need to free the existing error because it's only set if it's null.  So nothing to free.

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

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