Skip to content

8365526: Crash with null Symbol passed to SystemDictionary::resolve_or_null #28438

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+21 −10
Open

8365526: Crash with null Symbol passed to SystemDictionary::resolve_or_null #28438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6123316
8365526: Crash with null Symbol passed to SystemDictionary::resolve_o…
coleenp Nov 20, 2025
8e7be57
Reorder if statement and add an assert.
coleenp Nov 21, 2025
8d30c17
Fix typo.
coleenp Nov 22, 2025
c959cfe
I like this version better. Unconditionally set the nest_host_error …
coleenp Nov 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/hotspot/share/classfile/resolutionErrors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ void ResolutionErrorTable::add_entry(const constantPoolHandle& pool, int cp_inde

ResolutionErrorKey key(pool(), cp_index);
ResolutionErrorEntry *entry = new ResolutionErrorEntry(message);
_resolution_error_table->put(key, entry);
bool created = false;
_resolution_error_table->put_if_absent(key, entry, &created);
assert(created, "should be created not updated");
Comment on lines +88 to +90
Copy link
Member

@iklam iklam Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@coleenp coleenp Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I use that, I lose the ability to assert that the new entry was created and not already found, which I really want.

}

// find entry in the table
Expand Down Expand Up @@ -126,6 +128,15 @@ ResolutionErrorEntry::~ResolutionErrorEntry() {
}
}

void ResolutionErrorEntry::set_nest_host_error(const char* message) {
// If a message is already set, free it.
if (nest_host_error() != nullptr) {
FREE_C_HEAP_ARRAY(char, _nest_host_error);
}
_nest_host_error = message;
}


class ResolutionErrorDeleteIterate : StackObj {
ConstantPool* p;

Expand Down
7 changes: 2 additions & 5 deletions src/hotspot/share/classfile/resolutionErrors.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2005, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -91,10 +91,7 @@ class ResolutionErrorEntry : public CHeapObj<mtClass> {
~ResolutionErrorEntry();

// The incoming nest host error message is already in the C-Heap.
void set_nest_host_error(const char* message) {
_nest_host_error = message;
}

void set_nest_host_error(const char* message);

Symbol* error() const { return _error; }
const char* message() const { return _message; }
Expand Down
11 changes: 7 additions & 4 deletions src/hotspot/share/classfile/systemDictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,14 +1864,17 @@ void SystemDictionary::add_nest_host_error(const constantPoolHandle& pool,
{
MutexLocker ml(Thread::current(), SystemDictionary_lock);
ResolutionErrorEntry* entry = ResolutionErrorTable::find_entry(pool, which);
if (entry != nullptr && entry->nest_host_error() == nullptr) {
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 should be resolved if there is no entry");
ResolutionErrorTable::add_entry(pool, which, message);
} else {
// An existing entry means we had a true resolution failure (LinkageError) with our nest host, but we
// still want to add the error message for the higher-level access checks to report. We should
// only reach here under the same error condition, so we can ignore the potential race with setting
// the message. If we see it is already set then we can ignore it.
// the message, and set it again.
entry->set_nest_host_error(message);
Copy link
Member

@iklam iklam Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <<<<<<<<<<
    }
  }
}

Copy link
Contributor

@jdksjolen jdksjolen Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

      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:

// 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:

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.

Copy link
Contributor

@jdksjolen jdksjolen Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a ticket for this: 8372373

Copy link
Contributor Author

@coleenp coleenp Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@coleenp coleenp Nov 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my change, the case that ignores setting the nest_host_error should free it for now. Yes, I think this should be improved further.

} else {
ResolutionErrorTable::add_entry(pool, which, message);
}
}
}
Expand Down