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

[Artem Semenov]

On Thu, 21 Aug 2025 12:09:38 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> ```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.
>
>> 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 and confirmed by a specialist from another organization. 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.

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

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