RFR: 8346914: UB issue in scalbnA

[Joe Darcy]

On Tue, 10 Jun 2025 03:36:10 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 126:
> 
>> 124:   if (u_k <= (unsigned)-54) {
>> 125:     if (n > 50000)      /* in case integer overflow in n+k */
>> 126:       return hugeX*copysignA(hugeX,x);  /*overflow*/
> 
> This case (`n > 50000`) isn't possible with my suggest change to limit the usage scope for `u_k`, and
> can be deleted.

FYI, as an alternative, there is a Java-only implementation of scalb (and supporting functionality) in java.lang.Math that could be ported to C as another way to avoid this issue.

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

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