8346916: [REDO] align_up has potential overflow #23711
Conversation
|
👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into |
|
@caspernorrbin This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 273 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@albertnetymk, @kimbarrett, @dean-long) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@caspernorrbin The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
|
Can you explain what was wrong with the original fix? The BACKOUT only mentions that tests failed, but doesn't say why. |
The original fix failed because of tests where overflow was the expected result. In the files changed here, it was either possible to recover from the overflow, or the caller does their own error checking. In both cases, the caller relied on the previous behaviour from |
|
I don't see where we check the return value of align_up_or_min for the changes in src/hotspot/share/gc/shared/gcArguments.cpp. If tests fail because of align_up, maybe the test should be fixed? |
I share @dean-long concerns. This PR doesn't seem like the right direction. I think the earlier PR was correct, and |
For Besides that, I'm curious to hear what you feel is the "right direction". The other 3 cases here all account for the potential overflow. If it is possible to recover, I don't see the need to assert and crash. All other ~170 uses of |
|
I have reverted |
|
If MinHeapDeltaBytes is the only problem, maybe we should align it down the max heap size before trying to align it up, or give it a max size smaller than max_uintx. |
I think this makes more sense; maybe sth like |
|
Of the (previously modified) heap flags, The previous max of |
| @@ -190,7 +190,7 @@ bool PSOldGen::expand(size_t bytes) { | |||
| assert(bytes > 0, "precondition"); | |||
| #endif | |||
| const size_t alignment = virtual_space()->alignment(); | |||
| size_t aligned_bytes = align_up(bytes, alignment); | |||
| size_t aligned_bytes = align_up_or_min(bytes, alignment); | |||
There was a problem hiding this comment.
How about using bytes = MIN2(bytes, virtual_space()->uncommitted_size()) to dodge the potential overflow? I find it more intuitive to provide a proper arg to align_up and expect the result to be >= arg.
There was a problem hiding this comment.
Changed it to this now
There was a problem hiding this comment.
Thank you; my suggestion was insufficient... Need to have an early-return when uncommitted_size is 0 at the beginning of PSOldGen::expand. After this, I wonder if align_up_or_min is truly warranted.
There was a problem hiding this comment.
Added the extra check, tests should now work as expected!
I agree with your thoughts on align_up_or_min. Since it's no longer used, I went ahead and removed it. Now we. only have align_up_or_null left for pointer types.
| @@ -90,6 +92,16 @@ inline T* align_up(T* ptr, A alignment) { | |||
| return (T*)align_up((uintptr_t)ptr, alignment); | |||
| } | |||
|
|
|||
| template <typename T, typename A> | |||
| inline T* align_up_or_null(T* ptr, A alignment) { | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I forgot to add any description comments for can_align_up. And align_up description should say can_align_up
is a precondition.
There was a problem hiding this comment.
I removed align_up_or_null in favour of can_align_up, and I agree that it feels more usable.
Instead of align_up_or_null, we can now either return early or set a different value if can_align_up fails. I updated the files here and to me it feels like an improvement.
| @@ -190,7 +190,7 @@ bool PSOldGen::expand(size_t bytes) { | |||
| assert(bytes > 0, "precondition"); | |||
| #endif | |||
| const size_t alignment = virtual_space()->alignment(); | |||
| size_t aligned_bytes = align_up(bytes, alignment); | |||
| size_t aligned_bytes = can_align_up(bytes, alignment) ? align_up(bytes, alignment) : 0; | |||
There was a problem hiding this comment.
Why doesn't the previous revision using early-return + min2 work?
There was a problem hiding this comment.
That solution still works. With can_align_up and setting aligned_bytes to 0, we can use the logic that is already there a few lines down:
jdk/src/hotspot/share/gc/parallel/psOldGen.cpp
Lines 201 to 207 in caaf409
To me it felt better to continue to try to expand instead of aborting. If you prefer the other version I can swap back to it.
There was a problem hiding this comment.
try to expand instead of aborting.
Did the previous version abort? I thought the logic is essentially:
if (remaining == 0) {
return;
}
aligned_bytes = align_up(min2(bytes, remaining), alignment).
The below if (aligned_bytes == 0) { should not be reachable any more, since align-up-overflow can't occur.
There was a problem hiding this comment.
I meant aborting in the sense that we abort the expand operation by returning early instead of continuing.
If we don't that, the if (aligned_bytes == 0) { should be reachable with can_align_up if the align were to fail and we set aligned_bytes to 0. The remaining == 0 would then be caught later in the function inside expand_by(aligned_bytes).
I'll go ahead and revert to the other version, and remove the if (aligned_bytes == 0) { block as well.
| @@ -1420,7 +1420,7 @@ const int ObjectAlignmentInBytes = 8; | |||
| \ | |||
| product(size_t, MinHeapDeltaBytes, ScaleForWordSize(128*K), \ | |||
| "The minimum change in heap space due to GC (in bytes)") \ | |||
| range(0, max_uintx) \ | |||
| range(0, max_uintx / 2 + 1) \ | |||
There was a problem hiding this comment.
Since the option type is size_t the range should have s/max_uintx/SIZE_MAX/.
Though there are several other similar mismatches nearby. Perhaps leave tidying this
up to a followup change?
There was a problem hiding this comment.
Think it might be better to do them all at once in a followup. Right now, all size_t flags in the file use max_uintx for the max value instead of SIZE_MAX.
| } | ||
|
|
||
| template <typename T, typename A> | ||
| inline bool can_align_up(T* ptr, A alignment) { |
There was a problem hiding this comment.
Maybe this should be later in the file, near the other pointer variants?
Also, there's no need to parameterize the pointer type - just void* suffices.
There was a problem hiding this comment.
Moved it down just above the pointer-align_up, and removed the pointer template.
|
Do we really want to allow passing nullptr to can_align_up(void* ptr, A alignment)? |
There was a problem hiding this comment.
Do we really want to allow passing nullptr to can_align_up(void* ptr, A alignment)?
I don't see any problem with allowing or passing nullptr to can_align_up, align_up, or align_down. The result should be true and have no effect, as if the argument were the integer 0, right? Disallowing nullptr would introduce extra code in these functions, which would clutter the flow, in my opinion.
| 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)); | ||
| T mask = checked_cast<T>(alignment_mask(alignment)); | ||
| assert(size <= std::numeric_limits<T>::max() - mask, "overflow"); |
There was a problem hiding this comment.
Just curious, if can_align_up is precondition, why not assert(can_align_up(...), "precondition")? Then, the comment can even be dropped.
There was a problem hiding this comment.
That is probably a cleaner way to do it now that we have can_align_up. I swapped the assert, and changed the rest of the function to look like before.
|
I think it's fine to allow
|
|
Thank you everyone for the reviews! /integrate |
|
@caspernorrbin |
|
/sponsor |
|
Going to push as commit 86860ca.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@albertnetymk @caspernorrbin Pushed as commit 86860ca. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi everyone,
The
align_upfunction 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_upand found the places with explicit checks. Most notably, #23168 addedalign_up_or_nullto metaspace, but this function is also useful elsewhere. Given this, I relocated it toalign.hpp, alongside the rest of the alignment functions.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23711/head:pull/23711$ git checkout pull/23711Update a local copy of the PR:
$ git checkout pull/23711$ git pull https://git.openjdk.org/jdk.git pull/23711/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23711View PR using the GUI difftool:
$ git pr show -t 23711Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23711.diff
Using Webrev
Link to Webrev Comment