RFR: 8346866: [ASAN] memoryReserver.cpp reported applying non-zero offset to non-null pointer produced null pointer [v8]

[Axel Boldt-Christmas]

On Tue, 7 Jan 2025 12:28:59 GMT, SendaoYan <syan at openjdk.org> wrote:

>> Hi all,
>> This PR add an extra loop condition check `p2u(attach_point) > stepsize` in function `HeapReserver::Instance::try_reserve_range` to make sure the loop will not wrap around, and make UndefinedBehaviorSanitizer silent. The change do not change the original logic, risk is low.
>> 
>> Additional testing:
>> 
>> - [x]  jtreg tests(include tier1/2/3 etc., which include tests added by [PR22712](https://github.com/openjdk/jdk/pull/22712)) on linux-x64 with release build
>> - [x]  jtreg tests(include tier1/2/3 etc., which include tests added by [PR22712](https://github.com/openjdk/jdk/pull/22712)) on linux-x64 with fastdebug build
>> - [x]  jtreg tests(include tier1/2/3 etc., which include tests added by [PR22712](https://github.com/openjdk/jdk/pull/22712)) on linux-aarch64 with release build
>> - [x]  jtreg tests(include tier1/2/3 etc., which include tests added by [PR22712](https://github.com/openjdk/jdk/pull/22712)) on linux-aarch64 with fastdebug build
>
> SendaoYan has updated the pull request incrementally with one additional commit since the last revision:
> 
>   convert attach_point to size_t ang then convert to char* again, to avoid ubsan report "produced null pointer" fail.

I wonder if we could rewrite this where our attempts are more explicit. That is we do exactly the `num_attempts_to_try` and calculate the stepsize in such a way that we do not have problems with calculation addresses which are outside of the range.

Something along these lines. First I found it weird that the current implementation calculates the step size with the `+1` included when the range is inclusive. I instead divide it into the number of intervals and align it down so that the attatchment point calculation does not go outside the range `[lowest_start, highest_start]`. 
```C++
ReservedSpace HeapReserver::Instance::try_reserve_range(char *highest_start,
                                                        char *lowest_start,
                                                        size_t attach_point_alignment,
                                                        char *aligned_heap_base_min_address,
                                                        char *upper_bound,
                                                        size_t size,
                                                        size_t alignment,
                                                        size_t page_size) {
  assert(is_aligned(highest_start, attach_point_alignment), "precondition");
  assert(is_aligned(lowest_start, attach_point_alignment), "precondition");

  const size_t attach_range = highest_start - lowest_start;
  const uint64_t num_attempts_possible = (attach_range / attach_point_alignment) + 1;
  const uint64_t num_attempts_to_try   = MIN2((uint64_t)HeapSearchSteps, num_attempts_possible);
  const uint64_t num_intervals = num_attempts_to_try - 1;
  const size_t stepsize = num_intervals == 0 ? 0 : align_down(attach_range / num_intervals, attach_point_alignment);

  for (uint64_t i = 0; i < num_attempts_to_try; ++i) {
    char* const attach_point = highest_start - stepsize * i;
    ReservedSpace reserved = try_reserve_memory(size, alignment, page_size, attach_point);

    if (reserved.is_reserved()) {
      if (reserved.base() >= aligned_heap_base_min_address &&
          size <= (uintptr_t)(upper_bound - reserved.base())) {
        // Got a successful reservation.
        return reserved;
      }

      release(reserved);
    }
  }

  // Failed
  return {};
}

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

PR Comment: https://git.openjdk.org/jdk/pull/22897#issuecomment-2575518715