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

[Kim Barrett]

On Fri, 3 Jan 2025 12:53:21 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:
> 
>   Replace "(size_t) highest_start" instead as 1

Changes requested by kbarrett (Reviewer).

src/hotspot/share/memory/memoryReserver.cpp line 433:

> 431:   // At least one is possible even for 0 sized attach range.
> 432:   const uint64_t num_attempts_possible = (attach_range / attach_point_alignment) + 1;
> 433:   const uint64_t num_attempts_to_try   = MIN2((uint64_t)HeapSearchSteps, num_attempts_possible);

pre-existing, but github won't let me comment on line 429.  That pointer subtraction should be using
`pointer_delta(highest_start, lowest_start)`.

src/hotspot/share/memory/memoryReserver.cpp line 440:

> 438:   // Try reserve memory from top to bottom.
> 439:   for (size_t offset = attach_range;
> 440:        offset <= attach_range; // Avoid wrap around.

This comment is very misleading and confusing.  This code is *relying* on underflow and the resulting
wraparound of offset to end the iteration.  That's to prevent potential pointer arithmetic underflow later.

src/hotspot/share/memory/memoryReserver.cpp line 442:

> 440:        offset <= attach_range; // Avoid wrap around.
> 441:        offset -= stepsize) {
> 442:     ReservedSpace reserved = try_reserve_memory(size, alignment, page_size, lowest_start + offset);

I think I preferred the earlier version over this.  But I don't dislike this version enough to want to block it.
I liked the explicitness of the pointer-based iteration path, and am unbothered by the 2nd loop exit.

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

PR Review: https://git.openjdk.org/jdk/pull/22897#pullrequestreview-2533611427
PR Review Comment: https://git.openjdk.org/jdk/pull/22897#discussion_r1905045707
PR Review Comment: https://git.openjdk.org/jdk/pull/22897#discussion_r1905048670
PR Review Comment: https://git.openjdk.org/jdk/pull/22897#discussion_r1905053217