8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer

[sendaoYan]

Hi all,

Function 'os::attempt_reserve_memory_between(char*, char*, size_t, size_t, bool)' 'src/hotspot/share/runtime/os.cpp' reported "runtime error: applying non-zero offset to non-null pointer 0x000000001000 produced null pointer" by address sanitizer. Gtest in function 'os_attempt_reserve_memory_between_combos_vm_Test::TestBody' at file test/hotspot/gtest/runtime/test_os_reserve_between.cpp call 'os::attempt_reserve_memory_between (min=0x0, max=0x1000, bytes=4096, alignment=4096, randomize=true)' trigger this failure. Before this PR, the pointer var hi_end get value from max 0x1000, and then apply offset bytes, and max equals bytes, thus address sanitizer report this failure.

This PR change from hi_end < bytes to hi_end <= bytes will eliminate the undefined behaviour. Risk is low.

Additional testing:

• jtreg tests(which include tier1/2/3 etc.) on linux-x64
• jtreg tests(which include tier1/2/3 etc.) on linux-aarch64

---

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-8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer (Bug - P4)

Reviewers

• Stefan Karlsson (@stefank - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23508

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back syan! 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.

[openjdk]

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

8349554: [UBSAN] os::attempt_reserve_memory_between reported applying non-zero offset to non-null pointer produced null pointer

Reviewed-by: stefank, stuefe

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

• 7d52f1e: 8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees
• e9278de: 8348411: C2: Remove the control input of LoadKlassNode and LoadNKlassNode
• 5395ffa: 8327378: XMLStreamReader throws EOFException instead of XMLStreamException
• 1ed9ef1: 8349559: Compiler interface doesn't need to store protection domain
• f0ea38b: 8349509: [macos] Clean up macOS dead code in jpackage
• 7f6c687: 8349374: [JVMCI] concurrent use of HotSpotSpeculationLog can crash
• bd9b24c: 8349512: Duplicate PermittedSubclasses entries with doclint enabled
• b40f8ee: 8337251: C1: Improve Class.isInstance intrinsic
• 88a8483: 8349121: SSLParameters.setApplicationProtocols() ALPN example could be clarified
• fb847bb: 8349493: Replace sun.util.locale.ParseStatus usage with java.text.ParsePosition
• ... and 4 more: https://git.openjdk.org/jdk/compare/1eb54e4228ba9319ac2f980055ed366dd861ec0b...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]

@sendaoYan 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]

Webrevs

• 01: Full - Incremental (6430a35a)
• 00: Full (8dca2b00)

[stefank]

I'm not convinced that this is the nicest way to fix the issue. This change now makes it so that some of the addresses are typed as pointers and hi_end is typed as an integer. When doing these kind of changes I think it is important to take a step back and think about the types and see if there are ways to make them consistent in the changed functions.

I wonder if a change like:

- if ((uintptr_t)hi_end < bytes) {
+ if ((uintptr_t)hi_end <= bytes) {

Would silence the compiler as well? Given that it would filter away the problematic case that leads to a null pointer.

But even that compares an address with a range, which is also unfortunate (IMHO). So, I think it would be nicer to extract the max range and use that in the comparison:

uintptr_t max_range = hi_end - nullptr;
if (max_range <= bytes) {

Just to be a bit cleaner with the types.

Or maybe even use the lowest attach point instead of nullptr:

uintptr_t max_range = hi_end - lo_att;
if (max_range < bytes) {

and then we can keep < instead of <= because hi_end - bytes will then not evaluate to null.

[sendaoYan]

Would silence the compiler

- if ((uintptr_t)hi_end < bytes) {
+ if ((uintptr_t)hi_end <= bytes) {

Yes.

Or maybe even use the lowest attach point instead of nullptr:

uintptr_t max_range = hi_end - lo_att;
if (max_range < bytes) {

hi_end less than lo_att in some cases, hi_end - lo_att subtraction will overflow, and save a bigger value to max_range, so if (max_range < bytes) return false.

Should we change like below:

-  if ((uintptr_t)hi_end < bytes) {
+  uintptr_t max_range = hi_end - lo_att;
+  if (max_range < bytes || hi_end < lo_att) {

[stefank]

hi_end less than lo_att in some cases

Do you know if that only happens in our tests and not in normal JVM runs? If it is only in testing then it would be preferable to add preconditions to the function and then fix the testing to adhere to the preconditions.

With that said, an easy fix would be to just continue the tradition of the function and have early returns when this happens:

  if (hi_end < lo_att) {
    return nullptr; // no need to go on
  }

  uintptr_t max_range = hi_end - lo_att;
  if (max_range < bytes) {  

[sendaoYan]

hi_end less than lo_att in some cases

Do you know if that only happens in our tests and not in normal JVM runs?

It happens in normal JVM runs. The command is:

build/linux-x86_64-server-slowdebug/images/jdk/bin/java -Xlog:os+map=trace -Xshare:dump -XX:SharedArchiveFile=build/linux-x86_64-server-slowdebug/images/jdk/lib/server/classes_nocoops_coh.jsa -server -XX:-UseCompressedOops -XX:+UnlockExperimentalVMOptions -XX:+UseCompactObjectHeaders -Xmx128M -Xms128M -XX:+UseG1GC

The output snippet:

[0.020s][debug][os,map] reserve_between (range [0x0000000000000000-0x0000000000400000), size 0x40000000, alignment 0x1000000, randomize: 1)

The call trace:

#0  os::attempt_reserve_memory_between (min=0x0, max=0x400000, bytes=1073741824, alignment=16777216, randomize=true)
    at src/hotspot/share/runtime/os.cpp:2024
#1  0x00007fffecbf8ea0 in CompressedKlassPointers::reserve_address_space_X (from=0, to=4194304, size=1073741824, alignment=16777216, aslr=true) at src/hotspot/share/oops/compressedKlass.cpp:190
#2  0x00007fffecbf8f86 in CompressedKlassPointers::reserve_address_space_for_unscaled_encoding (size=1073741824, aslr=true) at src/hotspot/share/oops/compressedKlass.cpp:195
#3  0x00007fffecbfa66a in CompressedKlassPointers::reserve_address_space_for_compressed_classes (size=1073741824, aslr=true, optimize_for_zero_base=true)
    at src/hotspot/cpu/x86/compressedKlass_x86.cpp:37
#4  0x00007fffef10ff4f in Metaspace::reserve_address_space_for_compressed_classes (size=1073741824, optimize_for_zero_base=true) at src/hotspot/share/memory/metaspace.cpp:589
#5  0x00007fffef111580 in Metaspace::global_initialize () at src/hotspot/share/memory/metaspace.cpp:789
#6  0x00007ffff03d10c7 in universe_init () at src/hotspot/share/memory/universe.cpp:887

[sendaoYan]

But even that compares an address with a range, which is also unfortunate (IMHO). So, I think it would be nicer to extract the max range and use that in the comparison:

uintptr_t max_range = hi_end - nullptr;
if (max_range <= bytes) {

The expr uintptr_t max_range = hi_end - nullptr; will case gcc/ckang report compiler error: "invalid operands to binary expression ('char *const' and 'std::nullptr_t')"

So should change like:

- if ((uintptr_t)hi_end < bytes) {
+ if ((uintptr_t)hi_end <= bytes) {

[stefank]

Sure. Let's leave any potential type cleanups for the future.

[sendaoYan]

Sure. Let's leave any potential type cleanups for the future.

Okey, PR has been updated.

[tstuefe]

Good. Thanks for fixing this. @stefank thanks for looking at this.

[stefank]

Looks good. Thanks for fixing.

[stefank]

The following is not a request to change your PR. It's merely an explanation of what I think could be done to this function to get rid of some of the comparisons of bytes/ranges with pointers:

  char* lower_limit = MAX2(min, absolute_min);
  char* upper_limit = MIN2(max, absolute_max);

  // Precondition check
  if (lower_limit >= upper_limit) {
    return nullptr; // no need to go on
  }

  // Calculate first attach points
  assert(alignment_adjusted < std::numeric_limits<uintptr_t>::max() - (uintptr_t)upper_limit, "overflow precondition");
  char* const lo_att = align_up(lower_limit, alignment_adjusted);

  if (lo_att >= upper_limit) {
    // no attachment point within limits
    return nullptr;
  }

  if (bytes < size_t(upper_limit - lo_att)) {
    // no attachment point that can accommodate the request
    return nullptr;
  }

  // Now we are guaranteed to have an attachment point that can
  // accommodate the request
  char* hi_att = align_down(upper_limit - bytes, alignment_adjusted);
  assert(hi_att >= lo_att, "checked above");

This is completely untested

[sendaoYan]

Thanks @stefank @tstuefe for the suggestions and reviews.

The additional test has finish and no new failure.

/integrate

[openjdk]

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

• 7d52f1e: 8349525: RBTree: provide leftmost, rightmost, and a simple way to print trees
• e9278de: 8348411: C2: Remove the control input of LoadKlassNode and LoadNKlassNode
• 5395ffa: 8327378: XMLStreamReader throws EOFException instead of XMLStreamException
• 1ed9ef1: 8349559: Compiler interface doesn't need to store protection domain
• f0ea38b: 8349509: [macos] Clean up macOS dead code in jpackage
• 7f6c687: 8349374: [JVMCI] concurrent use of HotSpotSpeculationLog can crash
• bd9b24c: 8349512: Duplicate PermittedSubclasses entries with doclint enabled
• b40f8ee: 8337251: C1: Improve Class.isInstance intrinsic
• 88a8483: 8349121: SSLParameters.setApplicationProtocols() ALPN example could be clarified
• fb847bb: 8349493: Replace sun.util.locale.ParseStatus usage with java.text.ParsePosition
• ... and 4 more: https://git.openjdk.org/jdk/compare/1eb54e4228ba9319ac2f980055ed366dd861ec0b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@sendaoYan Pushed as commit 8f6ccde.

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