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 2 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
2 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
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
5 changes: 4 additions & 1 deletion src/hotspot/share/classfile/systemDictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1870,7 +1870,10 @@ void SystemDictionary::add_nest_host_error(const constantPoolHandle& pool,
// 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.
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 <<<<<<<<<<
    }
  }
}

} else {
} 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);
Copy link
Contributor

@tkrodriguez tkrodriguez Nov 21, 2025

Choose a reason for hiding this comment

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

I might be inclined to swap the cases.

if (entry == nullptr) {
    ...
} else if (entry->nest_host_error() == nullptr) {
    ...
}

Is there ever a situation where replacing an entry in ResolutionErrorTable is correct? Maybe there should be a check for that somewhere?

Copy link
Contributor Author

@coleenp coleenp Nov 21, 2025

Choose a reason for hiding this comment

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

Yes, this reorganization would look nicer.
No, there's a never a situation where calling replacing an entry in the ResolutionErrorTable is correct because this HashTable::put() function leaks the value that it has replaced. I've been testing an assert for this.
In general, this function can leak the value but I did a test and we don't leak anything but this one right now. But I think we should fix this separately.

}
}
Expand Down