RFR: 8346914: UB issue in scalbnA

[Kim Barrett]

On Thu, 5 Jun 2025 07:48:03 GMT, David Holmes <dholmes 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.

I think I've come up with a simpler solution.  See detailed comments.

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.

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 isn't possible with my suggest change to limit the usage scope for `u_k`, and can be deleted.

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

Changes requested by kbarrett (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/25656#pullrequestreview-2911807382
PR Review Comment: https://git.openjdk.org/jdk/pull/25656#discussion_r2136803034
PR Review Comment: https://git.openjdk.org/jdk/pull/25656#discussion_r2136848223