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

[Andrew Dinn]

On Thu, 21 Aug 2025 13:18:26 GMT, Artem Semenov <asemenov at openjdk.org> wrote:

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

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 abut 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 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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26798#discussion_r2291265263