RFR: 8281213: Unsafe uses of long and size_t in MemReporterBase::diff_in_current_scale

[Evgeny Astigeevich]

On Mon, 5 Dec 2022 13:54:57 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

Changes requested by eastigeevich (Committer).

src/hotspot/share/services/memReporter.hpp line 82:

> 80:       amount -= _scale / 2;
> 81:     }
> 82:     return amount / _scale;

I think the correct code should be like the following:

bool is_negative = false;
if (s1 < s2) {
  is_negative = true;
  swap(s1, s2);
}

size_t amount = s1 - s2;
assert(amount <= SIZE_MAX - _scale / 2, "size_t overflow");
amount = (amount + _scale / 2) / _scale;
if (is_negative) {
  assert(amount - 1  <= LONG_MAX, "cannot fit scaled diff into long);
  return (long)(-(long long)amount);
} else {
  assert(amount <= LONG_MAX, "cannot fit scaled diff into long);
  return (long)amount;
}

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

PR: https://git.openjdk.org/jdk/pull/11514