Skip to content

8365604: Null pointer dereference in src/hotspot/share/adlc/output_h.cpp ArchDesc::declareClasses() #26798

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

Closed
wants to merge 9 commits into from
Closed

8365604: Null pointer dereference in src/hotspot/share/adlc/output_h.cpp ArchDesc::declareClasses() #26798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3fdf834
8365604 Null pointer dereference in src/hotspot/share/adlc/output_h.c…
savoptik Jun 9, 2025
556e998
The same issue is present in src/hotspot/share/opto/vectorIntrinsics.…
savoptik Jul 28, 2025
66d02f9
The same issue is present in src/hotspot/share/jfr/periodic/sampling/…
savoptik Aug 5, 2025
ccab0f2
The same issue is present in src/hotspot/share/nmt/mallocSiteTable.cp…
savoptik Aug 6, 2025
06264cd
The same issue is present in src/hotspot/share/c1/c1_LinearScan.cpp I…
savoptik Aug 7, 2025
ba82be3
The same issue is present in src/hotspot/share/jvmci/jvmciEnv.cpp JVM…
savoptik Aug 6, 2025
80777ce
The same issue is present in src/hotspot/share/runtime/continuationFr…
savoptik Aug 13, 2025
9e10c16
Update src/hotspot/share/adlc/output_h.cpp
savoptik Aug 21, 2025
dd21148
Update src/hotspot/share/c1/c1_LinearScan.cpp
savoptik Aug 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
1 change: 1 addition & 0 deletions src/hotspot/share/adlc/output_h.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1949,6 +1949,7 @@ void ArchDesc::declareClasses(FILE *fp) {
fprintf(fp, " };\n");
}*/
else if( instr->is_ideal_copy() &&
(instr->_matrule != nullptr && instr->_matrule->_rChild != nullptr) &&
!strcmp(instr->_matrule->_lChild->_opType,"stackSlotP") ) {
// !!!!!
// Special hack for ideal Copy of pointer. Bottom type is oop or not depending on input.
Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/c1/c1_LinearScan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4418,7 +4418,8 @@ Interval* Interval::split(int split_pos) {
}
assert(cur != Range::end(), "split interval after end of last range");

if (cur->from() < split_pos) {
if ((cur != nullptr) &&
(cur->from() < split_pos)) {
result->_first = new Range(split_pos, cur->to(), cur->next());
cur->set_to(split_pos);
cur->set_next(Range::end());
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,15 +470,15 @@ void JfrThreadSampler::set_period(bool is_java_period, int64_t period_millis) {

void JfrThreadSampler::set_java_sample_period(int64_t period_millis) {
assert(period_millis >= 0, "invariant");
if (_instance == nullptr && 0 == period_millis) {
if (_instance == nullptr || 0 == period_millis) {
return;
}
instance().set_period(true, period_millis);
}

void JfrThreadSampler::set_native_sample_period(int64_t period_millis) {
assert(period_millis >= 0, "invariant");
if (_instance == nullptr && 0 == period_millis) {
if (_instance == nullptr || 0 == period_millis) {
return;
}
instance().set_period(false, period_millis);
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/jvmci/jvmciEnv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ JVMCICompileState::JVMCICompileState(CompileTask* task, JVMCICompiler* compiler)
_jvmti_can_access_local_variables = JvmtiExport::can_access_local_variables() ? 1 : 0;
_jvmti_can_post_on_exceptions = JvmtiExport::can_post_on_exceptions() ? 1 : 0;
_jvmti_can_pop_frame = JvmtiExport::can_pop_frame() ? 1 : 0;
assert(_task != nullptr, "The task cannot be null");
_target_method_is_old = _task != nullptr && _task->method()->is_old();
if (task->is_blocking()) {
task->set_blocking_jvmci_compile_state(this);
Expand Down
5 changes: 3 additions & 2 deletions src/hotspot/share/nmt/mallocSiteTable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,9 @@ MallocSite* MallocSiteTable::malloc_site(uint32_t marker) {
MallocSiteHashtableEntry* head = _table[bucket_idx];
for (size_t index = 0;
index < pos_idx && head != nullptr;
index++, head = (MallocSiteHashtableEntry*)head->next()) {}
assert(head != nullptr, "Invalid position index");
index++, head = ((MallocSiteHashtableEntry*)head->next() == nullptr) ? head :
(MallocSiteHashtableEntry*)head->next()) {}
Comment on lines -171 to +172
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right to me. We check head != nullptr in the loop condition so we cannot reach the assignment if it is null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A situation is possible where head becomes nullptr when head->next() returns nullptr on the last iteration. Then, after the loop finishes, assert(head != nullptr) will trigger (only in debug mode), and return head->data() will cause a program error

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is it possible?

Perhaps you could explain how pos_idx is being used in this loop to guard against that happening and why that does not make this safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

head->next() returns a pointer to _next without any checks.

In turn, the _next pointer is marked as volatile, which means it can be modified at any moment, for example, in another thread.

From this, I conclude that a check in this location is desirable. Moreover, pos_idx is also not being checked. It is quite possible that head->next() could turn out to be nullptr.

But I don’t mind. If you are sure that there can’t be a nullptr in this place, I will withdraw this patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, pos_idx is also not being checked

I don't know what you mean by this comment. pos_idx is being checked in the loop test before the call to head->next() in that same test.

The important question you need to address is why and what that check guarantees. I say you need to address it because you are the one claiming that there is a possible nullptr dereference here without any evidence that it has occurred in practice. If that is based on a correct analysis of the code then you need to explain how we can arrive at a situtation where we hit a null pointer that takes into account the logic of the loop test. So far you have not done so.

n.b. I am not claiming there is no possibility of a nullptr dereference here (although I can form my own opinion). I'm asking you to tell me why I should take your claim that it is possible seriously. Your answers so far are not convincing me that you have understood how this code works.

Copy link
Contributor Author

@savoptik savoptik Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

pos_idx receives its value when calling a certain function pos_idx_from_marker(marker), and there is no check before the loop to ensure that it is within the bounds of the _table size.

I mentioned above that I am not insisting on this particular patch. This issue was detected by a static analyzer. After that, based on my limited knowledge, I considered it confirmed… If you have any refutation, please share your thoughts. In that case, I will revert this patch and mark the trigger as “NO FIX REQUIRED”.

As far as I have checked, there are no checks anywhere in the calls to this function to compare the marker with the table or any other entities in any form.

I certainly do not claim to understand this code as well as you or any other member of the hotspot team.

Copy link
Contributor

@adinn adinn Aug 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Well, this leads right to the root of the problem I have with this report. As you say, pos_idx does indeed come out of a marker object. It took me about a minute to identify that this marker object is created in the function that sits right above the one your code assistant flagged as problematic -- even though I am not at all familiar with this code. It looks clear to me that, given the right call sequence for calls that create a marker and then consume it here, the check on pos_idx will ensure that we don't drop off the end of the list with a null pointer. So, it looks very liek this code has been designed so that the presence of a marker with a suitable pos_idx is intended to ensure this loop terminates before that happens. I am sure someone in this project knows whether that is the case but it is not you or your coding assistant.

I'm not suggesting that that calling sequence is actually right and that the check for pos_idx will definitely avoid dropping off the end. Indeed, I would welcome a bug report that proved it to be wrong. However, what is clear that both you and your coding assistant have failed to appreciate how some relatively obvious parts of this design actually operate. That renders your (or your tool's) analysis a shallow and unhelpful distraction; using it as an excuse to raise a purported 'issue' in the absence of any evidence of an actual issue is very much a waste of time for this project's reviewers.

Your error is compounded by the fact that you (or more likely your coding assistant) are suggesting changes which, because they are not founded in a correct understanding of the design, could potentially lead to worse outcomes than the speculative nullptr dereference they are intended to remedy -- as I explained when discussing your change to the control flow logic in the ALDC code. So, not only is this report unhelpful it is potentially harmful.

Ultimately the takeaway here is that the OpenJDK bug system is not here to report, review and add patches to remedy issues that you or your code assistant tool invents on the basis of misinformed assumptions. It is here to report, review and add patches to remedy issues that can be shown to actually affect the correct operation of the JVM and JDK,either by a reproducible test or by well-reasoned argument. So, please do not continue to spam the project with bug reports like this simply because a potentially bogus patch will improve your experience with what is clearly a decidedly fallible tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this leads right to the root of the problem I have with this report. As you say, pos_idx does indeed come out of a marker object. It took me about a minute to identify that this marker object is created in the function that sits right above the one your code assistant flagged as problematic -- even though I am not at all familiar with this code. It looks clear to me that, given the right call sequence for calls that create a marker and then consume it here, the check on pos_idx will ensure that we don't drop off the end of the list with a null pointer. So, it looks very liek this code has been designed so that the presence of a marker with a suitable pos_idx is intended to ensure this loop terminates before that happens. I am sure someone in this project knows whether that is the case but it is not you or your coding assistant.

I'm not suggesting that that calling sequence is actually right and that the check for pos_idx will definitely avoid dropping off the end. Indeed, I would welcome a bug report that proved it to be wrong. However, what is clear that both you and your coding assistant have failed to appreciate how some relatively obvious parts of this design actually operate. That renders your (or your tool's) analysis a shallow and unhelpful distraction; using it as an excuse to raise a purported 'issue' in the absence of any evidence of an actual issue is very much a waste of time for this project's reviewers.

Your error is compounded by the fact that you (or more likely your coding assistant) are suggesting changes which, because they are not founded in a correct understanding of the design, could potentially lead to worse outcomes than the speculative nullptr dereference they are intended to remedy -- as I explained when discussing your change to the control flow logic in the ALDC code. So, not only is this report unhelpful it is potentially harmful.

Ultimately the takeaway here is that the OpenJDK bug system is not here to report, review and add patches to remedy issues that you or your code assistant tool invents on the basis of misinformed assumptions. It is here to report, review and add patches to remedy issues that can be shown to actually affect the correct operation of the JVM and JDK,either by a reproducible test or by well-reasoned argument. So, please do not continue to spam the project with bug reports like this simply because a potentially bogus patch will improve your experience with what is clearly a decidedly fallible tool.

I’m sorry to have taken up your time.


return head->data();
}

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/opto/vectorIntrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1313,7 +1313,7 @@ bool LibraryCallKit::inline_vector_gather_scatter(bool is_scatter) {
const TypeAryPtr* arr_type = addr_type->isa_aryptr();

// The array must be consistent with vector type
if (arr_type == nullptr || (arr_type != nullptr && !elem_consistent_with_arr(elem_bt, arr_type, false))) {
if (arr_type != nullptr && !elem_consistent_with_arr(elem_bt, arr_type, false)) {
log_if_needed(" ** not supported: arity=%d op=%s vlen=%d etype=%s atype=%s ismask=no",
is_scatter, is_scatter ? "scatter" : "gather",
num_elem, type2name(elem_bt), type2name(arr_type->elem()->array_element_basic_type()));
Comment on lines -1316 to 1319
Copy link
Member

Choose a reason for hiding this comment

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

There is a bug here but I'm not sure it is what you think it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addr_type->isa_aryptr(); might return nullptr, while in elem_consistent_with_arr(elem_bt, arr_type, false), arr_type is only checked with an assert.
Moreover, the presence of a check in the original version indicates that arr_type can be null, and there is no protection against this.

Expand Down
3 changes: 2 additions & 1 deletion src/hotspot/share/runtime/continuationFreezeThaw.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1047,7 +1047,8 @@ freeze_result FreezeBase::finalize_freeze(const frame& callee, frame& caller, in
assert(!allocated_old_in_freeze_fast || (!UseZGC && !UseG1GC), "Unexpected allocation");

DEBUG_ONLY(bool empty_chunk = true);
if (unextended_sp < _freeze_size || chunk->is_gc_mode() || (!allocated_old_in_freeze_fast && chunk->requires_barriers())) {
if (unextended_sp < _freeze_size || (chunk != nullptr &&
(chunk->is_gc_mode() || (!allocated_old_in_freeze_fast && chunk->requires_barriers())))) {
// ALLOCATE NEW CHUNK

if (lt.develop_is_enabled()) {
Expand Down