RFR: 8346914: UB issue in scalbnA

[Kim Barrett]

On Tue, 17 Jun 2025 07:14:24 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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.

Here is the diff for what I'm suggesting.

diff --git a/src/hotspot/share/runtime/sharedRuntimeMath.hpp b/src/hotspot/share/runtime/sharedRuntimeMath.hpp
index 91dda2a4fe8..321f3be580a 100644
--- a/src/hotspot/share/runtime/sharedRuntimeMath.hpp
+++ b/src/hotspot/share/runtime/sharedRuntimeMath.hpp
@@ -111,16 +111,23 @@ static double scalbnA(double x, int n) {
     if (n< -50000) return tiny*x;       /*underflow*/
   }
   if (k==0x7ff) return x+x;             /* NaN or Inf */
-  k = k+n;
-  if (k > 0x7fe) return hugeX*copysignA(hugeX,x); /* overflow  */
+  // If the high (sign) bit of u_k is set, then either
+  // n is positive and k+n would overflow, or
+  // n is negative and |n| > k.
+  unsigned u_k = (unsigned)k + (unsigned)n;
+  if ((u_k > 0x7fe) && (n > 0)) {
+    // Either (k+n > 0x7fe && k+n <= INT_MAX) or k+n would overflow.
+    return hugeX*copysignA(hugeX,x); /* overflow  */
+  }
+  // Set k to k+n, now that we know k+n wouldn't overflow.
+  // We know that k+n <= (int)0x7fe, and might be negative if n is negative.
+  k = (int)u_k;
   if (k > 0) {                          /* normal result */
     set_high(&x, (hx&0x800fffff)|(k<<20));
     return x;
   }
   if (k <= -54) {
-    if (n > 50000)      /* in case integer overflow in n+k */
-      return hugeX*copysignA(hugeX,x);  /*overflow*/
-    else return tiny*copysignA(tiny,x); /*underflow*/
+    return tiny*copysignA(tiny,x); /*underflow*/
   }
   k += 54;                              /* subnormal result */
   set_high(&x, (hx&0x800fffff)|(k<<20));

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

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