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

[Andrew Dinn]

On Fri, 15 Aug 2025 11:58:48 GMT, Artem Semenov <asemenov at openjdk.org> wrote:

> The defect has been detected and confirmed in the function ArchDesc::declareClasses() located in the file src/hotspot/share/adlc/output_h.cpp with static code analysis. This defect can potentially lead to a null pointer dereference.
> 
> The pointer instr->_matrule is dereferenced in line 1952 without checking for nullptr, although earlier in line 1858 the same pointer is checked for nullptr, which indicates that it can be null.
> 
> According to [this](https://github.com/openjdk/jdk/pull/26002#issuecomment-3023050372) comment, this PR contains fixes for similar cases in other places.

I'm not clear that the original issue is necessarily a bug that needs fixing with a skip to the next else if case.

The justification for adding `instr->_matrule != nullptr && instr->_matrule->_rChild != nullptr` to the if branch test is that earlier code allows for the possibility that `instr->_matrule` might be null. However, that check is performed in an unconditional context for any value of `instr` whereas this specific else branch limits the circumstance to the case where `instr->is_ideal_copy()` is found to be true.

So, the prior test offers no guarantee that in this restricted case a null pointer should or should not be possible. The original design may assume that a successful test for `instr->is_ideal_copy()` ought to guarantee that both `instr->_matrule` and `instr->_matrule->_rChild` are non-null. That cannot be determined by the evidence offered. It can only be determined by looking at how instr is constructed.

So, rather than just skip to the next case we might need to handle this with an assert and fix whatever code is producing an ideal copy with null fields.

Given the level of analysis offered for this case I am suspicious as to whether the other cases tacked onto this issue ought to be accepted at face value without some justification as to why a null check and skip to the next case is correct.

I'm also wondering how and why all these cases and associated amendments were arrived at? Is this perhaps based on output from a code analysis tool (perhaps even a genAI tool). If so then I think 1) we ought to be told and 2) we ought to treat its recommendations with a very healthy dose of skepticism.

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

PR Comment: https://git.openjdk.org/jdk/pull/26798#issuecomment-3206613521