RFR: 8346914: UB issue in scalbnA

[David Holmes]

On Tue, 10 Jun 2025 02:34:04 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> This fixes address a problem with signed integer overflow in the C fdlibm scalbnA function.
>> 
>> Testing this code is extremely difficult. First, the only time this code will get executed is if intrinsics have been disabled by `-XX:-InlineIntrinsics`. Second, finding the math routines and the arguments thereto which actually reach this function is also difficult. I have found 3 tests only that hit the `scalbnA` function at the point where the potential overflow occurs, but beyond that I cannot determine what arguments will cause the different code paths to be taken. Consequently the only testing I could do here was to make a copy of the original `scalbnA` function and then place a check in the callers that the old and new code produced the same result. Again how much coverage this actually gave is not known. That test code still remains in the PR as the initial commit.
>> 
>> Due to the testing problem this test relies on detailed code inspection and analysis, so here are the changes and the reasoning for them:
>> 
>>     // Convert to unsigned to avoid signed integer overflow
>> [1] unsigned u_k = ((unsigned) k) + n;
>> 
>> [2] if (u_k > 0x7fe && u_k <= 0x7fffffff) return hugeX*copysignA(hugeX,x); /* overflow */
>> [3] if (u_k > 0 && u_k <= 0x7fe) { /* normal result */
>> [4]   set_high(&x, (hx&0x800fffff)|((k+n)<<20));
>>       return x;
>>     }
>> 
>> [5] if (u_k <= (unsigned)-54) {
>>       if (n > 50000) /* in case integer overflow in n+k */
>>         return hugeX*copysignA(hugeX,x); /*overflow*/
>>       else return tiny*copysignA(tiny,x); /*underflow*/
>>     }
>> [6] k = u_k + 54; /* subnormal result */
>>     set_high(&x, (hx&0x800fffff)|(k<<20));
>>     return x*twom54;
>> 
>> 
>> [1] We use an unsigned variable, `u_k`, for the potentially overflowing addition
>> 
>> [2] We check the value of `u_k` adjusting the bounds to emulate a signed-int range
>> 
>> [3] Again we check `u_k` and adjust the range
>> 
>> [4] We know `k+n` is in range so we use that directly. I didn't use `u_k` here because I didn't want to have to reason about whether the use of an unsigned type would change anything in the expression
>> 
>> [5] We check if `u_k` is logically less than what -54 would be
>> 
>> [6] We bring `u_k` back into positive range by adding 54 and then store safely into `k`
>> 
>> Thanks.
>
> src/hotspot/share/runtime/sharedRuntimeMath.hpp line 118:
> 
>> 116:   unsigned u_k = ((unsigned) k) + n;
>> 117: 
>> 118:   if (u_k > 0x7fe && u_k <= 0x7fffffff) return hugeX*copysignA(hugeX,x); /* overflow  */
> 
> I think `(unsigned)INT_MAX` would be more explicit about what's going on.
> This is also starting to push my limits on sufficiently simple to be a one-line `if`, and even more-so with my
> suggested change.
> I note that this isn't distinguishing between (1) `n > 0` and `k + n` overflows and wraps around to negative
> `int` vs (2) `n < 0` and `k + n` is negative. And that makes later code (both pre-existing and changed)
> harder to understand. I _think_ better here would be `u_k > 0x7fe && n > 0` => overflow, with some later
> adjustments. Then, if the test fails and we're not huge, `k = (int)u_k;` and use `k` as before, dropping
> `u_k`, so discarding the remainder of the currently proposed changes.

I'm not seeing the full suggestion here. In the original code this line:

if (k > 0x7fe) return hugeX*copysignA(hugeX,x); /* overflow  */

is defining a logical overflow, not the wrapping overflow that we are trying to deal with. The wrapping overflow results in a negative value, which is the third case that gets handled. So AFAICS we need to use `u_k` all the way through until the end.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25656#discussion_r2151503322