RFR: 8332506: SIGFPE In ObjectSynchronizer::is_async_deflation_needed()

[Coleen Phillimore]

On Thu, 19 Dec 2024 01:13:36 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> This PR solves a division by zero problem, that according to the bug report happened in `ObjectSynchronizer::is_async_deflation_needed()`. As it turns out it really happened in the inlined  `monitors_used_above_threshold()` function. The problematic line looked like this:
>> 
>> `size_t monitor_usage = (monitors_used * 100LL) / ceiling;`
>> 
>> Unfortunately the `ceiling` value was increased every time there where too many deflations without any progress. This whould eventually lead to an overflow in the `ceiling` value, and in unlucky circumstances, it would become zero. Thus causing a division by zero crash.
>> 
>> This PR makes sure not to increase the `ceiling` value if `monitor_usage` is below the `MonitorUsedDeflationThreshold`.
>> It also makes sure the ceiling value is never zero, and does not wrap around.
>> 
>> Tested okay in tier1-5 and `test/hotspot/jtreg/runtime/Monitor/MonitorUsedDeflationThresholdTest.java`
>
> src/hotspot/share/runtime/synchronizer.cpp line 1281:
> 
>> 1279:       _no_progress_cnt >= NoAsyncDeflationProgressMax) {
>> 1280:     double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
>> 1281:     size_t new_ceiling = ceiling + (size_t)((double)ceiling * remainder) + 1;
> 
> if you are looking fort the minimal fix for the division-by-zero problem, then I think simply fixing this line to avoid overflow will suffice:
> 
> size_t delta = (size_t)(ceiling * remainder) + 1;
> if (ceiling > SIZE_MAX - delta) { // overflow
>   ceiling = SIZE_MAX;  // Or some other positive limit
> } else {
>   ceiling += delta;
> }

I think this code with the introduction of delta looks a bit more clear to me, if you use it in your existing patch to replace lines 1286 to 1289.  That would still be a minimal fix.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22815#discussion_r1891871576