RFR: 8346916: [REDO] align_up has potential overflow [v5]

[Kim Barrett]

On Mon, 3 Mar 2025 16:57:13 GMT, Casper Norrbin <cnorrbin at openjdk.org> wrote:

>> Hi everyone,
>> 
>> The `align_up` function can potentially overflow, resulting in undefined behavior. Most use cases rely on the assumption that aligned_result >= original. To address this, I've added an assertion to verify this condition.
>> 
>> The original PR (#20808) missed cases where overflow checks already existed, so I've now went through usages of `align_up` and found the places with explicit checks. Most notably, #23168 added `align_up_or_null` to metaspace, but this function is also useful elsewhere. Given this, I relocated it to `align.hpp`, alongside the rest of the alignment functions.
>
> Casper Norrbin has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - removed align_up_or_min test from test_align
>  - psoldgen check + removed align_up_or_min

Changes requested by kbarrett (Reviewer).

src/hotspot/share/utilities/align.hpp line 96:

> 94: 
> 95: template <typename T, typename A>
> 96: inline T* align_up_or_null(T* ptr, A alignment) {

Rather than specialized align_up variants, I think better and more generally
usable would be a predicate for testing whether align_up is safe.  Something
like a `bool can_align_up(value, alignment)` function.

I'm happy to see align_up_or_min removed from this PR for that reason, as well
as because it has usability issues.  Is a min-valued return indicating failure
to align?  Or was it just the argument is min-valued and already aligned?
Consider passing an unsigned zero value as the value to be aligned.

This seems to work:

diff --git a/src/hotspot/share/utilities/align.hpp b/src/hotspot/share/utilities/align.hpp
index b67e61036a0..d73e8e086ca 100644
--- a/src/hotspot/share/utilities/align.hpp
+++ b/src/hotspot/share/utilities/align.hpp
@@ -30,6 +30,8 @@
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/powerOfTwo.hpp"
+
+#include <limits>
 #include <type_traits>
 
 // Compute mask to use for aligning to or testing alignment.
@@ -70,6 +72,17 @@ constexpr T align_down(T size, A alignment) {
   return result;
 }
 
+template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
+constexpr bool can_align_up(T size, A alignment) {
+  return align_down(std::numeric_limits<T>::max(), alignment) >= size;
+}
+
+template<typename A>
+inline bool can_align_up(const void* p, A alignment) {
+  static_assert(sizeof(p) == sizeof(uintptr_t), "assumption");
+  return can_align_up(reinterpret_cast<uintptr_t>(p), alignment);
+}
+
 template<typename T, typename A, ENABLE_IF(std::is_integral<T>::value)>
 constexpr T align_up(T size, A alignment) {
   T adjusted = checked_cast<T>(size + alignment_mask(alignment));
diff --git a/test/hotspot/gtest/utilities/test_align.cpp b/test/hotspot/gtest/utilities/test_align.cpp
index 3c03fd5f24d..2112d43feab 100644
--- a/test/hotspot/gtest/utilities/test_align.cpp
+++ b/test/hotspot/gtest/utilities/test_align.cpp
@@ -27,6 +27,51 @@
 #include "unittest.hpp"
 
 #include <limits>
+#include <type_traits>
+
+template<typename T, typename A>
+static constexpr void test_can_align_up() {
+  int alignment_value = 4;
+  int small_value = 63;
+  A alignment = static_cast<A>(alignment_value);
+
+  EXPECT_TRUE(can_align_up(static_cast<T>(small_value), alignment));
+  EXPECT_TRUE(can_align_up(static_cast<T>(-small_value), alignment));
+  EXPECT_TRUE(can_align_up(std::numeric_limits<T>::min(), alignment));
+  EXPECT_FALSE(can_align_up(std::numeric_limits<T>::max(), alignment));
+  EXPECT_FALSE(can_align_up(std::numeric_limits<T>::max() - 1, alignment));
+  EXPECT_TRUE(can_align_up(align_down(std::numeric_limits<T>::max(), alignment), alignment));
+  EXPECT_FALSE(can_align_up(align_down(std::numeric_limits<T>::max(), alignment) + 1, alignment));
+  if (std::is_signed<T>::value) {
+    EXPECT_TRUE(can_align_up(static_cast<T>(-1), alignment));
+    EXPECT_TRUE(can_align_up(align_down(static_cast<T>(-1), alignment), alignment));
+    EXPECT_TRUE(can_align_up(align_down(static_cast<T>(-1) + 1, alignment), alignment));
+  }
+}
+
+TEST(Align, test_can_align_up_int32_int32) {
+  test_can_align_up<int32_t, int32_t>();
+}
+
+TEST(Align, test_can_align_up_uint32_uint32) {
+  test_can_align_up<uint32_t, uint32_t>();
+}
+
+TEST(Align, test_can_align_up_int32_uint32) {
+  test_can_align_up<int32_t, uint32_t>();
+}
+
+TEST(Align, test_can_align_up_uint32_int32) {
+  test_can_align_up<uint32_t, int32_t>();
+}
+
+TEST(Align, test_can_align_up_ptr) {
+  uint alignment = 4;
+  char buffer[8];
+
+  EXPECT_TRUE(can_align_up(buffer, alignment));
+  EXPECT_FALSE(can_align_up(reinterpret_cast<void*>(UINTPTR_MAX), alignment));
+}
 
 // A few arbitrarily chosen values to test the align functions on.
 static constexpr uint64_t values[] = {1, 3, 10, 345, 1023, 1024, 1025, 23909034, INT_MAX, uint64_t(-1) / 2, uint64_t(-1) / 2 + 100, uint64_t(-1)};

`align_up` can assert `can_align_up(size, alignment)`.

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

PR Review: https://git.openjdk.org/jdk/pull/23711#pullrequestreview-2650497091
PR Review Comment: https://git.openjdk.org/jdk/pull/23711#discussion_r1978995585