RFR: 8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer

[Stefan Karlsson]

On Fri, 7 Feb 2025 02:25:10 GMT, SendaoYan <syan at openjdk.org> wrote:

> Hi all,
> 
> Function 'os::attempt_reserve_memory_between(char*, char*, size_t, size_t, bool)' 'src/hotspot/share/runtime/os.cpp' reported "runtime error: applying non-zero offset to non-null pointer 0x000000001000 produced null pointer" by address sanitizer. Gtest in function 'os_attempt_reserve_memory_between_combos_vm_Test::TestBody'  at file test/hotspot/gtest/runtime/test_os_reserve_between.cpp call 'os::attempt_reserve_memory_between (min=0x0, max=0x1000, bytes=4096, alignment=4096, randomize=true)' trigger this failure. Before this PR, the pointer var `hi_end` get value from `max` 0x1000, and then apply offset `bytes`, and `max` equals `bytes`, thus address sanitizer report this failure.
> 
> This PR change the type of var `hi_end` from `char*` to `size_t` will eliminate the undefined behaviour, and do not change the original logic. Risk is low.
> 
> Additional testing:
> 
> - [ ] jtreg tests(which include tier1/2/3 etc.) on linux-x64
> - [ ] jtreg tests(which include tier1/2/3 etc.) on linux-aarch64

I'm not convinced that this is the nicest way to fix the issue. This change now makes it so that some of the addresses are typed as pointers and `hi_end` is typed as an integer. When doing these kind of changes I think it is important to take a step back and think about the types and see if there are ways to make them consistent in the changed functions.

I wonder if a change like:

- if ((uintptr_t)hi_end < bytes) {
+ if ((uintptr_t)hi_end <= bytes) {


Would silence the compiler as well? Given that it would filter away the problematic case that leads to a null pointer.

But even that compares an address with a range, which is also unfortunate (IMHO). So, I think it would be nicer to extract the max range and use that in the comparison:


uintptr_t max_range = hi_end - nullptr;
if (max_range <= bytes) {


Just to be a bit cleaner with the types.

Or maybe even use the lowest attach point instead of nullptr:

uintptr_t max_range = hi_end - lo_att;
if (max_range < bytes) {


and then we can keep `<` instead of `<=` because `hi_end - bytes` will then not evaluate to null.

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

Changes requested by stefank (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23508#pullrequestreview-2601143572