8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer #19597

MBaesken · 2024-06-07T11:52:42Z

When running :tier1 hs tests, the following issue has been reported when running with ubsan enabled binaries (configure flag --enable-ubsan)

stderr: [/jdk/src/hotspot/share/cds/archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer
#0 0x7f33db52823f in ArchiveHeapLoader::init_mapped_heap_info(unsigned char*, long, int) /jdk/src/hotspot/share/cds/archiveHeapLoader.cpp:70
#1 0x7f33dc6d3ad4 in FileMapInfo::map_heap_region_impl() /jdk/src/hotspot/share/cds/filemap.cpp:2211
#2 0x7f33dc6d4ba4 in FileMapInfo::map_heap_region() /jdk/src/hotspot/share/cds/filemap.cpp:2129
#3 0x7f33dc6d52a7 in FileMapInfo::map_or_load_heap_region() /jdk/src/hotspot/share/cds/filemap.cpp:2019
#4 0x7f33ddaf7fdb in MetaspaceShared::map_archives(FileMapInfo*, FileMapInfo*, bool) /jdk/src/hotspot/share/cds/metaspaceShared.cpp:1183
#5 0x7f33ddaf8f54 in MetaspaceShared::initialize_runtime_shared_and_meta_spaces() /jdk/src/hotspot/share/cds/metaspaceShared.cpp:943
#6 0x7f33ddadd60f in Metaspace::global_initialize() /jdk/src/hotspot/share/memory/metaspace.cpp:714
#7 0x7f33de9bc7f9 in universe_init() /jdk/src/hotspot/share/memory/universe.cpp:866
#8 0x7f33dcc93691 in init_globals() /jdk/src/hotspot/share/runtime/init.cpp:128
#9 0x7f33de92a720 in Threads::create_vm(JavaVMInitArgs*, bool*) /jdk/src/hotspot/share/runtime/threads.cpp:553
#10 0x7f33dd02d477 in JNI_CreateJavaVM_inner /jdk/src/hotspot/share/prims/jni.cpp:3581
#11 0x7f33dd02d477 in JNI_CreateJavaVM /jdk/src/hotspot/share/prims/jni.cpp:3672
#12 0x7f33e42a90e5 in InitializeJVM /jdk/src/java.base/share/native/libjli/java.c:1550
#13 0x7f33e42a90e5 in JavaMain /jdk/src/java.base/share/native/libjli/java.c:491
#14 0x7f33e42b2748 in ThreadJavaMain /jdk/src/java.base/unix/native/libjli/java_md.c:642
#15 0x7f33e42616e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73)
#16 0x7f33e391550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: f732026552f6adff988b338e92d466bc81a01c37)

Seems that CompressedOops::base() can be nullptr , so adding to it some non-zero value triggers ubsan because of undefined behavior. In the JBS-bug there was already a little discussion and it seems disabling this warning might be an option.

otherwise some workaround like

template<typename T>
T* add_to_ptr_maybe_null(T* ptr, uintptr_t val) {
  return (T*)((uintptr_t)ptr + val * sizeof(T));
}

could be used .

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-8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer (Bug - P4)

Reviewers

Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 0d89b783
Lutz Schmidt (@RealLucy - Reviewer) ⚠️ Review applies to 0d89b783

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19597

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-07T11:53:13Z

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

openjdk · 2024-06-07T11:54:30Z

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

8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer

Reviewed-by: stuefe, lucy

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

b818679: 8293980: Resolve CONSTANT_FieldRef at CDS dump time
eb2488f: 8330198: Add some class loading related perf counters to measure VM startup
c4702ca: 8333827: JDK 23 RDP1 L10n resource files update
bb7ef03: 8333599: Improve description of \b matcher in j.u.r.Pattern
0721dbe: 8334032: javax.print: Missing @SInCE tag in new class OutputBin
6462b87: 8333685: Make update-copyright-year script more useful
b09a451: 8333840: C2 SuperWord: wrong result for MulAddS2I when inputs permuted
cff048c: 8334162: Gatherer.defaultCombiner has an erronous @see-link
814cf87: 8313562: hsperfdata should export module path and "launcher" metadata
9ed8629: 8333801: Typos in @code references of BufferedImage and JTableHeader
... and 71 more: https://git.openjdk.org/jdk/compare/40b2fbd8207404961d3d23375b288cceafc3f902...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-07T11:54:59Z

@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-07T11:57:39Z

Webrevs

01: Full - Incremental (a39100b4)
00: Full (0d89b783)

dholmes-ora · 2024-06-12T03:09:55Z

src/hotspot/share/cds/archiveHeapLoader.cpp

@@ -61,6 +61,10 @@ ptrdiff_t ArchiveHeapLoader::_mapped_heap_delta = 0;

 // Every mapped region is offset by _mapped_heap_delta from its requested address.
 // See FileMapInfo::heap_region_requested_address().
+// avoid adding to CompressedOops::base() == nullptr


I don't understand what the comment is trying to say.

This is surely a bug in ubsan as I should be able to take any address, including zero, and add an offset to get another address!

Hi David, it is no ubsan-bug, see also here for more details about this warning https://reviews.llvm.org/D67122 .

tstuefe · 2024-06-12T06:13:40Z

@dholmes-ora Adding to NULL is undefined, Matthias is right there. E.g. I once worked on machines that tracked the validity of pointers in hardware. Adding anything to NULL would give you a hardware interrupt.

However, that is not the case for any of our platforms. Which is why Matthias mutes UBSAN here.

tstuefe

Looks good.

But could you add a define for this, please? Would be better to read. We already have ATTRIBUTE_NO_ASAN (see share/sanitizers). I think a corresponding ATTRIBUTE_NO_UBSAN would be good.

Such a define would also be the central place to switch on Visual Studio if they ever add ubsan support.

MBaesken · 2024-06-12T06:52:48Z

Looks good.

But could you add a define for this, please? Would be better to read. We already have ATTRIBUTE_NO_ASAN (see share/sanitizers). I think a corresponding ATTRIBUTE_NO_UBSAN would be good.

Such a define would also be the central place to switch on Visual Studio if they ever add ubsan support.

Thanks for the review !
Introducing ATTRIBUTE_NO_UBSAN sounds like a good idea, we could reuse that in vmerror.cpp too (function crash_with_sigfpe).
Where should I place the macro , maybe a separate header sanitizers/ub.hpp next to sanitizers/address.hpp ?
On the other hand, those defines would maybe be needed also outside HS in the JDK libs.

MBaesken · 2024-06-12T07:03:12Z

Thomas, could we do the ATTRIBUTE_NO_UBSAN in a follow up ?

tstuefe · 2024-06-12T08:14:04Z

Looks good.
But could you add a define for this, please? Would be better to read. We already have ATTRIBUTE_NO_ASAN (see share/sanitizers). I think a corresponding ATTRIBUTE_NO_UBSAN would be good.
Such a define would also be the central place to switch on Visual Studio if they ever add ubsan support.

Thanks for the review ! Introducing ATTRIBUTE_NO_UBSAN sounds like a good idea, we could reuse that in vmerror.cpp too (function crash_with_sigfpe). Where should I place the macro , maybe a separate header sanitizers/ub.hpp next to sanitizers/address.hpp ? On the other hand, those defines would maybe be needed also outside HS in the JDK libs.

ubsan.hpp?

tstuefe · 2024-06-12T08:14:22Z

Thomas, could we do the ATTRIBUTE_NO_UBSAN in a follow up ?

Sure.

RealLucy

LGTM.

dholmes-ora · 2024-06-13T02:46:59Z

Adding to NULL is undefined,

Hmm okay. So really our address abstraction should be dealing with intptr_t or uintptr_t to perform any arithmetic and then cast the result back to an actual pointer type when done?

tstuefe · 2024-06-13T04:57:20Z

Adding to NULL is undefined,

Hmm okay. So really our address abstraction should be dealing with intptr_t or uintptr_t to perform any arithmetic and then cast the result back to an actual pointer type when done?

Well, not sure.

C99:

5 An integer may be converted to any pointer type. Except as previously specified, the result is implementation-defined, might not be correctly aligned, might not point to an entity of the referenced type, and might be a trap representation. 56)

The one platform I talked about (IBM AS/400 in native mode) did the latter. Casting integer to pointer resulted in an invalid pointer that would trap on dereferencing.

Personally, I think hunting UB is sorting out the chaff to find the useful bits. Pity that we cannot selectivly switch off UB cases with the compiler, and have to decorate code instead.

MBaesken · 2024-06-13T15:48:55Z

Pity that we cannot selectivly switch off UB cases with the compiler, and have to decorate code instead.

We can switch off whole ubsan problem/issue 'classes' (and do this already for some types of shifts). But this is of course not fine granular if done for the whole codebase.

Btw any objections integrating this now (and doing in the follow up the ATTRIBUTE_NO_UBSAN macro, see https://bugs.openjdk.org/browse/JDK-8334239) ?

tstuefe · 2024-06-13T19:35:43Z

Pity that we cannot selectivly switch off UB cases with the compiler, and have to decorate code instead.

We can switch off whole ubsan problem/issue 'classes' (and do this already for some types of shifts). But this is of course not fine granular if done for the whole codebase.

Btw any objections integrating this now (and doing in the follow up the ATTRIBUTE_NO_UBSAN macro, see https://bugs.openjdk.org/browse/JDK-8334239) ?

no problem, but i would remove the comment about avoiding adding to null.

MBaesken · 2024-06-14T07:13:03Z

I removed the comment.
Thanks for the reviews !

/integrate

openjdk · 2024-06-14T07:13:23Z

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

b818679: 8293980: Resolve CONSTANT_FieldRef at CDS dump time
eb2488f: 8330198: Add some class loading related perf counters to measure VM startup
c4702ca: 8333827: JDK 23 RDP1 L10n resource files update
bb7ef03: 8333599: Improve description of \b matcher in j.u.r.Pattern
0721dbe: 8334032: javax.print: Missing @SInCE tag in new class OutputBin
6462b87: 8333685: Make update-copyright-year script more useful
b09a451: 8333840: C2 SuperWord: wrong result for MulAddS2I when inputs permuted
cff048c: 8334162: Gatherer.defaultCombiner has an erronous @see-link
814cf87: 8313562: hsperfdata should export module path and "launcher" metadata
9ed8629: 8333801: Typos in @code references of BufferedImage and JTableHeader
... and 71 more: https://git.openjdk.org/jdk/compare/40b2fbd8207404961d3d23375b288cceafc3f902...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-14T07:13:29Z

@MBaesken Pushed as commit 6861766.

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

JDK-8332818

0d89b78

openjdk bot changed the title ~~JDK-8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer~~ 8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer Jun 7, 2024

openjdk bot added the rfr Pull request is ready for review label Jun 7, 2024

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

dholmes-ora reviewed Jun 12, 2024

View reviewed changes

tstuefe approved these changes Jun 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 12, 2024

MBaesken mentioned this pull request Jun 12, 2024

8333887: ubsan: unsafe.cpp:247:13: runtime error: store to null pointer of type 'volatile int' #19630

Closed

3 tasks

RealLucy approved these changes Jun 12, 2024

View reviewed changes

remove comment

a39100b

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

openjdk bot closed this Jun 14, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer #19597

8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer #19597

MBaesken commented Jun 7, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024 •

edited

Loading

openjdk bot commented Jun 7, 2024

mlbridge bot commented Jun 7, 2024 •

edited

Loading

dholmes-ora Jun 12, 2024

MBaesken Jun 12, 2024

tstuefe commented Jun 12, 2024

tstuefe left a comment

MBaesken commented Jun 12, 2024

MBaesken commented Jun 12, 2024

tstuefe commented Jun 12, 2024

tstuefe commented Jun 12, 2024

RealLucy left a comment

dholmes-ora commented Jun 13, 2024

tstuefe commented Jun 13, 2024

MBaesken commented Jun 13, 2024

tstuefe commented Jun 13, 2024

MBaesken commented Jun 14, 2024

openjdk bot commented Jun 14, 2024

openjdk bot commented Jun 14, 2024

8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer #19597

8332818: ubsan: archiveHeapLoader.cpp:70:27: runtime error: applying non-zero offset 18446744073707454464 to null pointer #19597

Conversation

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

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024 • edited Loading

openjdk bot commented Jun 7, 2024

mlbridge bot commented Jun 7, 2024 • edited Loading

Webrevs

dholmes-ora Jun 12, 2024

Choose a reason for hiding this comment

MBaesken Jun 12, 2024

Choose a reason for hiding this comment

tstuefe commented Jun 12, 2024

tstuefe left a comment

Choose a reason for hiding this comment

MBaesken commented Jun 12, 2024

MBaesken commented Jun 12, 2024

tstuefe commented Jun 12, 2024

tstuefe commented Jun 12, 2024

RealLucy left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jun 13, 2024

tstuefe commented Jun 13, 2024

MBaesken commented Jun 13, 2024

tstuefe commented Jun 13, 2024

MBaesken commented Jun 14, 2024

openjdk bot commented Jun 14, 2024

openjdk bot commented Jun 14, 2024

MBaesken commented Jun 7, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 7, 2024 •

edited

Loading

mlbridge bot commented Jun 7, 2024 •

edited

Loading