RFR: 8351334: [ubsan] memoryReserver.cpp:552:60: runtime error: applying non-zero offset 1073741824 to null pointer [v3]

[Afshin Zafari]

On Fri, 12 Sep 2025 09:01:18 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   lowest can be equal to highest
>
> src/hotspot/share/gc/shared/jvmFlagConstraintsGC.cpp line 288:
> 
>> 286:   // If an overflow happened in Arguments::set_heap_size(), MaxHeapSize will have too large a value.
>> 287:   // Check for this by ensuring that MaxHeapSize plus the requested min base address still fit within max_uintx.
>> 288:   if (value + MaxHeapSize < MaxHeapSize) {// overflow
> 
> Can we perform the overflow check via subtraction from the max value instead? I know that unsigned types do wrap around, but at a first glance this will look wrong and make the reader stop and think.
> 
> Style: Space between { and //
> 
> 
> if (std::numeric_limits<size_t>::max() - value < MaxHeapSize) { // overflow

Using ` a < a - b` for checking overflow, does not work when `a - b` is negative and it will become a very large number when cast to unsigned.

Space added.

> src/hotspot/share/memory/memoryReserver.cpp line 560:
> 
>> 558:   if (!FLAG_IS_DEFAULT(HeapBaseMinAddress)) {
>> 559:     reserved = try_reserve_memory(size + noaccess_prefix, alignment, page_size, (char *)aligned_heap_base_min_address);
>> 560:     if (reserved.base() != (char *)aligned_heap_base_min_address) { // Enforce this exact address.
> 
> Style: Please hug the * to the char for the casts

Done.
There are some preexisting cases like this. Should I change them too?

> src/hotspot/share/memory/memoryReserver.cpp line 584:
> 
>> 582:       // Calc address range within we try to attach (range of possible start addresses).
>> 583:       char* const highest_start = align_down((char *)UnscaledOopHeapMax - size, attach_point_alignment);
>> 584:       char* const lowest_start  = align_up((char *)aligned_heap_base_min_address, attach_point_alignment);
> 
> Keep these as uintptr_t instead and only cast when trying to reserve

Done.

> src/hotspot/share/memory/memoryReserver.cpp line 595:
> 
>> 593: 
>> 594:     // Give it several tries from top of range to bottom.
>> 595:     if (aligned_heap_base_min_address + size <= (uintptr_t)zerobased_max && // Zerobased theoretical possible.
> 
> Do the opposite: Have zerobased_max be uintptr_t and only cast it when it needs to become a char*

Done.

> src/hotspot/share/memory/memoryReserver.cpp line 612:
> 
>> 610:       }
>> 611:       lowest_start = align_up(lowest_start, attach_point_alignment);
>> 612:       assert(lowest_start <= highest_start, "lowest: " INTPTR_FORMAT " highest: " INTPTR_FORMAT,
> 
> Keep these as uintptr_t and cast to char* only when reserving

done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26955#discussion_r2348276821
PR Review Comment: https://git.openjdk.org/jdk/pull/26955#discussion_r2348278478
PR Review Comment: https://git.openjdk.org/jdk/pull/26955#discussion_r2348278713
PR Review Comment: https://git.openjdk.org/jdk/pull/26955#discussion_r2348279035
PR Review Comment: https://git.openjdk.org/jdk/pull/26955#discussion_r2348288660