8332865: ubsan: os::attempt_reserve_memory_between reports overflow #19543

MBaesken · 2024-06-04T15:19:11Z

When running by ubsan-enabled binaries on Linux x86_64, os::attempt_reserve_memory_between reports overflows.
This happens in the :tier1 tests ( gtest/LargePageGtests_use-large-pages.jtr )

"runtime error: pointer index expression with base 0x000000001000 overflowed to 0xfffffffffffff000"

This coding triggers the ubsan issue

  char* const hi_att = align_down(MIN2(max, absolute_max) - bytes, alignment_adjusted);
  if (hi_att > max) {
    return nullptr; // overflow
  }

However the function already contains overflow handling, so probably it is sufficient to add an attribute to the function os::attempt_reserve_memory_between to disable ubsan checks for this function.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8332865: ubsan: os::attempt_reserve_memory_between reports overflow (Bug - P4)

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
Christoph Langer (@RealCLanger - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19543/head:pull/19543
$ git checkout pull/19543

Update a local copy of the PR:
$ git checkout pull/19543
$ git pull https://git.openjdk.org/jdk.git pull/19543/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19543

View PR using the GUI difftool:
$ git pr show -t 19543

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19543.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-04T15:20:11Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

tstuefe

LGTM. Thanks for taking care of this.

openjdk · 2024-06-04T16:27:40Z

@MBaesken 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:

8332865: ubsan: os::attempt_reserve_memory_between  reports overflow

Reviewed-by: stuefe, clanger

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 47 new commits pushed to the master branch:

880c6b4: 8333178: ubsan: jvmti_tools.cpp:149:16: runtime error: null pointer passed as argument 2, which is declared to never be null
c7d2841: 8332070: Convert package.html files in java.management to package-info.java
7ef2831: 8333644: C2: assert(is_Bool()) failed: invalid node class: Phi
8f07853: 8333410: [AArch64] Clean unused classes in nativeInst_aarch64.hpp
6f690a5: 8333177: Invalid value used for enum Cell in ciTypeFlow::get_start_state
b351b5f: 8331736: C2: Live Node limit exceeded limit after JDK-8316991
b3f540d: 8332921: Ctrl+C does not call shutdown hooks after JLine upgrade
3089412: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
ca93907: 8256828: ostream::print_cr() truncates buffer in copy-through case
60ea17e: 8311177: Switching to interpreter only mode in carrier thread can lead to crashes
... and 37 more: https://git.openjdk.org/jdk/compare/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2...master

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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-06-04T16:49:06Z

@MBaesken The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2024-06-04T16:56:20Z

Webrevs

01: Full - Incremental (71c91621)
00: Full (983bda19)

stefank

Why is this better than fixing the overflow that causes the undefined behavior? IIUC, the current overflow checks is causing UB and that allows the compiler to do whatever, for example skip the return?

kimbarrett

Are you sure about the triggering code? I don't see anything in that snippet
that might overflow. The error message in JBS says the failure in on line
1928, but that doesn't look right either.

The calculation of lo_att seems like it could potentially overflow, and the
check is commented as being for overflow.

kimbarrett · 2024-06-05T04:35:26Z

src/hotspot/share/runtime/os.cpp

@@ -1889,6 +1889,9 @@ static void hemi_split(T* arr, unsigned num) {

 // Given an address range [min, max), attempts to reserve memory within this area, with the given alignment.
 // If randomize is true, the location will be randomized.
+#if defined(__clang__) || defined(__GNUC__)
+__attribute__((no_sanitize("undefined")))
+#endif


I don't think this attribute addition should be made. I agree with @stefank that we should be eliminating the
potential overflow, since the compiler (without ubsan) is within its rights to discard a pointer overflow check,
since pointer overflow is UB.

From what I saw and got info from Thomas, my impression was that the method contains already overflow handling.
But if you think the current overflow handling is not 'good enough' , maybe it needs improvement.

Yeah, I am more confused now than I was initially :) If you provide us with the requested info, maybe we can shed light onto this one.

I know that I spend a lot of time with this code, making sure all corner cases are clean. Therefore I am curious what ubsan digged up.

tstuefe · 2024-06-05T06:05:47Z

Hmm, this is partly my fault. It's my code, and Matthias asked me before whether I could spot any error, which I could not, so I adviced him to mute ubsan.

I added the callstack Matthias sent me to the JBS issue.

Looking closely, the error line number makes no sense. Maybe it was not in mainline, but an older JDK version?

This function is called in a gtest (attempt_reserve_memory_between_combos) that tries every possible combination (even very unlikely) of range boundaries,alignments,allocation size.

ubsan says that the pointer that overflowed originally was 0x1000. The only address that can be 0x1000 around the failing line number is min. The lowest value for max is 0x2000 (min + smallest range size 0x1000).

@MBaesken : Can you please:

doublecheck the ubsan error line?
reproduce the problem, run the gtestlauncher manually and give us the output of-Xlog:os+map=debug? (You can just pass vm options to the gtestlauncher). That way, we know exactly which parameters the function was called with.

Also, on the box where we see the error, can you please add the print out for sysctl vm.mmap_min_addr?

MBaesken · 2024-06-05T11:59:22Z

I rebuilt with current sources from this morning. Here is the stack from this

src/hotspot/share/runtime/os.cpp:1938:34: runtime error: pointer index expression with base 0x000000001000 overflowed to 0xfffffffffffff000
#0 0x7fe1b98d8c56 in os::attempt_reserve_memory_between(char*, char*, unsigned long, unsigned long, bool) src/hotspot/share/runtime/os.cpp:1938
#1 0x7fe1b6c52620 in call_attempt_reserve_memory_between test/hotspot/gtest/runtime/test_os_reserve_between.cpp:69
#2 0x7fe1b6c58e15 in test_attempt_reserve_memory_between test/hotspot/gtest/runtime/test_os_reserve_between.cpp:108
#3 0x7fe1b6c5a3c3 in os_attempt_reserve_memory_between_combos_vm_Test::TestBody() test/hotspot/gtest/runtime/test_os_reserve_between.cpp:291
#4 0x7fe1ba961603 in testing::Test::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:2687
#5 0x7fe1ba961603 in testing::Test::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:2677
#6 0x7fe1ba961d0d in testing::TestInfo::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:2836
#7 0x7fe1ba992618 in testing::TestSuite::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:3015
#8 0x7fe1ba992618 in testing::TestSuite::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:2968
#9 0x7fe1ba9935e5 in testing::internal::UnitTestImpl::RunAllTests() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:5920
#10 0x7fe1ba95988f in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(), char const) /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:2670
#11 0x7fe1ba95988f in testing::UnitTest::Run() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/src/gtest.cc:5484
#12 0x7fe1b65e4d27 in RUN_ALL_TESTS() /sapmnt/sapjvm_work/openjdk/tools/gtest/googletest-1.14.0/googletest/include/gtest/gtest.h:2317
#13 0x7fe1b65e4d27 in runUnitTestsInner test/hotspot/gtest/gtestMain.cpp:290
#14 0x56268b2d17d8 in main test/hotspot/gtest/gtestLauncher.cpp:40
#15 0x7fe1b064624c in __libc_start_main (/lib64/libc.so.6+0x3524c) (BuildId: f732026552f6adff988b338e92d466bc81a01c37)

MBaesken · 2024-06-05T12:05:04Z

btw. sysctl gives me
vm.mmap_min_addr = 65536

tstuefe · 2024-06-05T12:58:23Z

Okay, I re-run the ubsan test manually on my local x64 machine. With logging, and after squashing about a zillion unrelated ubsan errors, I see:

[0,311s][debug][os,map] reserve_between (range [0x0000000000000000-0x0000000000001000), size 0x2000, alignment 0x1000, randomize: 1)
/shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/runtime/os.cpp:1938:34: runtime error: pointer index expression with base 0x000000001000 overflowed to 0xfffffffffffff000
    #0 0x7f517dcda84e in os::attempt_reserve_memory_between(char*, char*, unsigned long, unsigned long, bool) /shared/projects/openjdk/jdk-jdk/source/src/hotspot/share/runtime/os.cpp:1938
    #1 0x7f517a83fbec in call_attempt_reserve_memory_between /shared/projects/openjdk/jdk-jdk/source/test/hotspot/gtest/runtime/test_os_reserve_between.cpp:69

So, the size of the mapping to be placed is larger than even the upper range boundary.

The fix is simple:

--- a/src/hotspot/share/runtime/os.cpp
+++ b/src/hotspot/share/runtime/os.cpp
@@ -1935,6 +1935,10 @@ char* os::attempt_reserve_memory_between(char* min, char* max, size_t bytes, siz
     return nullptr; // overflow
   }
 
+  char* const hi_end = MIN2(max, absolute_max);
+  if ((uintptr_t)hi_end < bytes) {
+    return nullptr; // no need to go on
+  }
   char* const hi_att = align_down(MIN2(max, absolute_max) - bytes, alignment_adjusted);
   if (hi_att > max) {
     return nullptr; // overflow

MBaesken · 2024-06-05T13:41:55Z

I just ran the test with -Xlog:os+map=debug and got the same debug output.

The fix is simple: ...

Thanks for suggesting the fix; should I just add this to the PR instead of disabling ubsan for the method ?

and after squashing about a zillion unrelated ubsan errors

Yeah there are unfortunately still a few ones remaining (I opened already JBS issues for most of them so the situation improves slowly) .

tstuefe · 2024-06-05T13:52:20Z

I just ran the test with -Xlog:os+map=debug and got the same debug output.

The fix is simple: ...

Thanks for suggesting the fix; should I just add this to the PR instead of disabling ubsan for the method ?

Yes, please just use this one. Disabling is not needed.

and after squashing about a zillion unrelated ubsan errors

Yeah there are unfortunately still a few ones remaining (I opened already JBS issues for most of them so the situation improves slowly) .

Good job in hunting those, this is useful work

tstuefe

Looks good. If checks are green and ubsan is happy, good to go.

MBaesken · 2024-06-06T12:26:03Z

Hi Christoph and Thomas, thanks for the reviews !

/integrate

openjdk · 2024-06-06T12:27:27Z

Going to push as commit 8de5d20.
Since your change was applied there have been 49 commits pushed to the master branch:

f15d423: 6942632: Hotspot should be able to use more than 64 logical processors on Windows
1a50bd0: 8322568: JFR: Improve metadata for IEEE rounding mode fields
880c6b4: 8333178: ubsan: jvmti_tools.cpp:149:16: runtime error: null pointer passed as argument 2, which is declared to never be null
c7d2841: 8332070: Convert package.html files in java.management to package-info.java
7ef2831: 8333644: C2: assert(is_Bool()) failed: invalid node class: Phi
8f07853: 8333410: [AArch64] Clean unused classes in nativeInst_aarch64.hpp
6f690a5: 8333177: Invalid value used for enum Cell in ciTypeFlow::get_start_state
b351b5f: 8331736: C2: Live Node limit exceeded limit after JDK-8316991
b3f540d: 8332921: Ctrl+C does not call shutdown hooks after JLine upgrade
3089412: 8326716: JVMTI spec: clarify what nullptr means for C/C++ developers
... and 39 more: https://git.openjdk.org/jdk/compare/8d3de45f4dfd60dc4e2f210cb0c085fcf6efb8e2...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-06T12:27:33Z

@MBaesken Pushed as commit 8de5d20.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8332865

983bda1

tstuefe approved these changes Jun 4, 2024

View reviewed changes

openjdk bot changed the title ~~JDK-8332865: ubsan: os::attempt_reserve_memory_between reports overflow~~ 8332865: ubsan: os::attempt_reserve_memory_between reports overflow Jun 4, 2024

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 4, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jun 4, 2024

stefank reviewed Jun 4, 2024

View reviewed changes

kimbarrett suggested changes Jun 5, 2024

View reviewed changes

MBaesken added 2 commits June 5, 2024 16:27

add check suggested by Thomas Stuefe

80e5e94

reuse hi_end

71c9162

tstuefe approved these changes Jun 5, 2024

View reviewed changes

RealCLanger approved these changes Jun 6, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 6, 2024

openjdk bot closed this Jun 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332865: ubsan: os::attempt_reserve_memory_between reports overflow #19543

8332865: ubsan: os::attempt_reserve_memory_between reports overflow #19543

MBaesken commented Jun 4, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 4, 2024

tstuefe left a comment

openjdk bot commented Jun 4, 2024 •

edited

Loading

openjdk bot commented Jun 4, 2024

mlbridge bot commented Jun 4, 2024 •

edited

Loading

stefank left a comment

kimbarrett left a comment

kimbarrett Jun 5, 2024

MBaesken Jun 5, 2024

tstuefe Jun 5, 2024

tstuefe commented Jun 5, 2024

MBaesken commented Jun 5, 2024 •

edited

Loading

MBaesken commented Jun 5, 2024

tstuefe commented Jun 5, 2024 •

edited

Loading

MBaesken commented Jun 5, 2024

tstuefe commented Jun 5, 2024

tstuefe left a comment

MBaesken commented Jun 6, 2024

openjdk bot commented Jun 6, 2024

openjdk bot commented Jun 6, 2024

8332865: ubsan: os::attempt_reserve_memory_between reports overflow #19543

8332865: ubsan: os::attempt_reserve_memory_between reports overflow #19543

Conversation

MBaesken commented Jun 4, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 4, 2024

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 4, 2024 • edited Loading

openjdk bot commented Jun 4, 2024

mlbridge bot commented Jun 4, 2024 • edited Loading

Webrevs

stefank left a comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett Jun 5, 2024

Choose a reason for hiding this comment

MBaesken Jun 5, 2024

Choose a reason for hiding this comment

tstuefe Jun 5, 2024

Choose a reason for hiding this comment

tstuefe commented Jun 5, 2024

MBaesken commented Jun 5, 2024 • edited Loading

MBaesken commented Jun 5, 2024

tstuefe commented Jun 5, 2024 • edited Loading

MBaesken commented Jun 5, 2024

tstuefe commented Jun 5, 2024

tstuefe left a comment

Choose a reason for hiding this comment

MBaesken commented Jun 6, 2024

openjdk bot commented Jun 6, 2024

openjdk bot commented Jun 6, 2024

MBaesken commented Jun 4, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 4, 2024 •

edited

Loading

mlbridge bot commented Jun 4, 2024 •

edited

Loading

MBaesken commented Jun 5, 2024 •

edited

Loading

tstuefe commented Jun 5, 2024 •

edited

Loading