RFR: 8331731: ubsan: relocInfo.cpp:155:30: runtime error: applying non-zero offset 18446744073709551614 to null pointer [v2]

[Vladimir Kozlov]

On Thu, 30 May 2024 21:53:15 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> Oh, and as it doesn't seem to have been clear from my earlier comments: I don't strongly oppose that you fix it this way you do in the RelocIterator, since I have very little interaction with that code.
>> 
>> The comment was more that I would prefer if we take a case-by-case approach when we look at other parts of HotSpot with similar problems and really think what the correct solution would be, and that we don't too quickly start to grab for the `add/sub_to_ptr` solution. Putting these functions in globalDefinitions makes it all too easy to just grab for these functions when we try to solve similar problems, IMHO. That's my 2c. I'm not blocking this patch, as long as we get somewhat decent names.
>
> My stance is the same as @stefank that I do not oppose this change to fix the immediate issue. 
> 
> Looking closer at how the `RelocIterator` is created from a `nmethod` it would never end up with a `nullptr - 1`. Because `relocation_begin()`, which is used to initialize `_current`, would never produce a nullptr. So there is no issue with the other constructor. So plugging the three holes above would remove the ub. (Along with introducing the invariant that you are not allowed to construct from a `CodeSection` with no relocations).
> 
>> But this is different issue for different RFE.
> 
> It may be a different RFE, but it is the same issue (unless I am misunderstanding you are referring to). The `!has_loc()` was specifically introduced to solve this exact ub bug. However it was the wrong property to check.  Reading #12854 gives me this impression as well. (Given that the logic around `has_loc` does not seem to have changed since 8153779ad32d1e8ddd37ced826c76c7aafc61894 )

> I agree with @xmas92, and I propose a slight change to his fix:

It does not matter. Few lines above we copied relocation info to new buffer and there is assert in `initialize_locs_from()`:

assert(this->locs_count() == source_cs->locs_count(), "sanity");

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

PR Comment: https://git.openjdk.org/jdk/pull/19424#issuecomment-2143068745