RFR: 8346914: UB issue in scalbnA

[Fredrik Bredberg]

On Thu, 5 Jun 2025 08:32:10 GMT, Axel Boldt-Christmas <aboldtch 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 124:
> 
>> 122:   }
>> 123: 
>> 124:   if (u_k <= (unsigned)-54) {
> 
> Could this just be? Or is this less clear / easier to miss?
> Suggestion:
> 
>   if (u_k <= -54u) {

For me it's less clear and far easier to miss.
I'll vote for `if (u_k <= (unsigned)-54) {`.
Reading `if (u_k <= -54u) {` just cooks my brain.

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

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