RFR: 8346916: [REDO] align_up has potential overflow [v3]
Albert Mingkun Yang
ayang at openjdk.org
Fri Feb 28 19:31:54 UTC 2025
On Fri, 28 Feb 2025 14:13:28 GMT, Casper Norrbin <cnorrbin at openjdk.org> wrote:
>> Hi everyone,
>>
>> The `align_up` function can potentially overflow, resulting in undefined behavior. Most use cases rely on the assumption that aligned_result >= original. To address this, I've added an assertion to verify this condition.
>>
>> The original PR (#20808) missed cases where overflow checks already existed, so I've now went through usages of `align_up` and found the places with explicit checks. Most notably, #23168 added `align_up_or_null` to metaspace, but this function is also useful elsewhere. Given this, I relocated it to `align.hpp`, alongside the rest of the alignment functions.
>>
>> Additionally, I've created `align_up_or_min`, which behaves similarly to the original align_up but handles overflows predictably across all integer types. This new function is used in the locations where overflow checks already exist, providing a safer alternative.
>
> Casper Norrbin has updated the pull request incrementally with one additional commit since the last revision:
>
> changed max size of MinHeapDeltaBytes
src/hotspot/share/gc/parallel/psOldGen.cpp line 193:
> 191: #endif
> 192: const size_t alignment = virtual_space()->alignment();
> 193: size_t aligned_bytes = align_up_or_min(bytes, alignment);
How about using `bytes = MIN2(bytes, virtual_space()->uncommitted_size())` to dodge the potential overflow? I find it more intuitive to provide a proper arg to `align_up` and expect the result to be >= arg.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23711#discussion_r1975917340
More information about the hotspot-dev
mailing list