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

[Stefan Karlsson]

On Fri, 7 Feb 2025 15:51:29 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 from `hi_end < bytes` to `hi_end <= bytes` will eliminate the undefined behaviour. 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
>
> SendaoYan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   hi_end should not less or equals to bytes.

The following is not a request to change your PR. It's merely an explanation of what I think could be done to this function to get rid of some of the comparisons of bytes/ranges with pointers:


  char* lower_limit = MAX2(min, absolute_min);
  char* upper_limit = MIN2(max, absolute_max);

  // Precondition check
  if (lower_limit >= upper_limit) {
    return nullptr; // no need to go on
  }

  // Calculate first attach points
  assert(alignment_adjusted < std::numeric_limits<uintptr_t>::max() - (uintptr_t)upper_limit, "overflow precondition");
  char* const lo_att = align_up(lower_limit, alignment_adjusted);

  if (lo_att >= upper_limit) {
    // no attachment point within limits
    return nullptr;
  }

  if (bytes < size_t(upper_limit - lo_att)) {
    // no attachment point that can accommodate the request
    return nullptr;
  }

  // Now we are guaranteed to have an attachment point that can
  // accommodate the request
  char* hi_att = align_down(upper_limit - bytes, alignment_adjusted);
  assert(hi_att >= lo_att, "checked above");


This is completely untested

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

PR Comment: https://git.openjdk.org/jdk/pull/23508#issuecomment-2643376373