RFR: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale [v4]
Evgeny Astigeevich
eastigeevich at openjdk.org
Wed Jan 11 10:07:22 UTC 2023
On Tue, 10 Jan 2023 11:27:43 GMT, Afshin Zafari <duke at openjdk.org> wrote:
>> ### Description
>> MemReporterBase::diff_in_current_scale is defined as follows:
>>
>> inline long diff_in_current_scale(size_t s1, size_t s2) const {
>> long amount = (long)(s1 - s2);
>> long scale = (long)_scale;
>> amount = (amount > 0) ? (amount + scale / 2) : (amount - scale / 2);
>> return amount / scale;
>> }
>>
>> Long and size_t can have different sizes: 4 bytes and 8 bytes (LLP64). The result of 's1 - s2' might not fit into long. It might not fit into int64_t. For example: s1 is SIZE_MAX and s2 is SIZE_MAX-MAX_INT64-1.
>>
>> Size_t should be used instead of long. Assertions must be added to check:
>> s1 >= s2 and (amount - scale/2) >= 0 and (amount + scale/2) <= SIZE_MAX.
>>
>> ### Patch
>> `long` is replaced by `size_t`. Comparison to 0 is implemented accordingly since size_t is always >= 0.
>> Since s1 can be less than s2 in some invocations of this method, no assert is written for `(s1 >= s2)` case.
>>
>> ### Test
>> local: runtime/NMT/Jcmd*
>> mach5: tier1
>
> Afshin Zafari has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
>
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
> - Merge branch 'master' into _8281213
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
> - 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale
src/hotspot/share/services/memReporter.hpp line 85:
> 83:
> 84: size_t amount = s1 - s2;
> 85: assert(amount <= SIZE_MAX - _scale / 2, "size_t overflow");
The assert `assert(amount <= SIZE_MAX - _scale / 2, "size_t overflow");` is not needed.
src/hotspot/share/services/memReporter.hpp line 88:
> 86: size_t scaled = (amount / _scale);
> 87: if ((amount % _scale) > (_scale - 1)/2) {
> 88: scaled += 1;
We need an assert `assert(scaled < SIZE_MAX, "size_t overflow");` for the case `_scale == 1`.
-------------
PR: https://git.openjdk.org/jdk/pull/11514
More information about the hotspot-runtime-dev
mailing list