8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero #19394

MBaesken · 2024-05-24T13:30:41Z

When running with ubsan enabled on Linux x86_64, I get in the HS :tier1 tests this error :

runtime/ErrorHandling/TestDwarf_dontCheckDecoder.jtr

/jdk/src/hotspot/share/utilities/vmError.cpp:2090:26: runtime error: division by zero
#0 0x7f16bc531f32 in crash_with_sigfpe /jdk/src/hotspot/share/utilities/vmError.cpp:2090
#1 0x7f16bc531f32 in VMError::controlled_crash(int) /jdk/src/hotspot/share/utilities/vmError.cpp:2137
#2 0x7f16bea2d8fd in JNI_CreateJavaVM_inner /jdk/src/hotspot/share/prims/jni.cpp:3621
#3 0x7f16bea2d8fd in JNI_CreateJavaVM /jdk/src/hotspot/share/prims/jni.cpp:3672
#4 0x7f16c5dbd0e5 in InitializeJVM /jdk/src/java.base/share/native/libjli/java.c:1550
#5 0x7f16c5dbd0e5 in JavaMain /jdk/src/java.base/share/native/libjli/java.c:491
#6 0x7f16c5dc6748 in ThreadJavaMain /jdk/src/java.base/unix/native/libjli/java_md.c:642
#7 0x7f16c5d756e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73)
#8 0x7f16c531550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: f732026552f6adff988b338e92d466bc81a01c37)

Reason is that we do a float division by zero to get a signal . This is desired by us so not really an error but ubsan cannot know this.
So add an attribute to this function that it has undefined behavior.
See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html (division by zero) . "Floating point division by zero. This is undefined per the C and C++ standards"

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

Warning

⚠️ Found leading lowercase letter in issue title for 8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero

Issue

JDK-8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero (Bug - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to c747e0c3
Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19394

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-24T13:31:05Z

👋 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-05-24T13:31:14Z

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

8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero

Reviewed-by: dholmes, 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 26 new commits pushed to the master branch:

72fbfe1: 8330577: G1 sometimes sends jdk.G1HeapRegionTypeChange for non-changes
0889155: 8332885: Clarify failure_handler self-tests
7f0ad51: 8332898: failure_handler: log directory of commands
617edf3: 8332497: javac prints an AssertionError when annotation processing runs on program with module imports
ffa4bad: 8332527: ZGC: generalize object cloning logic
a3a367e: 8332871: Parallel: Remove public bits APIs in ParMarkBitMap
61db2f5: 8079167: Fix documentation for G1SATBBufferEnqueueingThresholdPercent == 0
a083364: 8321292: SerialGC: NewSize vs InitialHeapSize check has an off-by-one error
16dba04: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null
4e8deb3: 8332922: Test java/io/IO/IO.java fails when /usr/bin/expect not exist
... and 16 more: https://git.openjdk.org/jdk/compare/6a35311468222f9335b43d548df2ecb80746b389...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-05-24T13:31:36Z

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

hotspot

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-05-24T13:34:24Z

Webrevs

01: Full - Incremental (b297f194)
00: Full (c747e0c3)

JohnTortugo · 2024-05-24T17:00:15Z

I'm wondering if we shouldn't follow the pattern of creating another header file in src/hotspot/share/sanitizers/ but otherwise LGTM. I'm not a reviewer, though.

kimbarrett · 2024-05-25T15:12:10Z

src/hotspot/share/utilities/vmError.cpp

+#endif
 static void ALWAYSINLINE crash_with_sigfpe() {

  // generate a native synchronous SIGFPE where possible;


Maybe simpler would be to change the definition to only use the divide-by-zero
approach for _WIN32 and always use the currently conditional fallback to
pthread_kill on non-_WIN32. Especially in light of the fact that the
divide-by-zero approach doesn't work on some platforms.

I also wonder if the comment about OSX incorrectly implementing raise is
correct? Maybe that's been fixed? Or maybe it's not a bug, but a BSD-ism?

I do not know about the OSX specific issues, maybe someone else can comment?
Regarding the handling on UNIX, Thomas commented and I think the coding should better stay.

macOS raise raises the signal to the process not the thread (per Posix requirements).

Hi David, so the comment

// OSX implements raise(sig) incorrectly so we need to // explicitly target the current thread

seems to be not correct, should we change it e.g. to your comment

// macOS raise raises the signal to the process not the thread (per Posix requirements) // so we need to explicitly target the current thread

The comment says raise is broken, it just doesn't say exactly how, though it is implied by the "we need to explicitly target the current thread".

It's not "broken". OSX/darwin is BSD-derived, and does not always follow POSIX.
https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man3/raise.3.html
"The raise() function sends the signal sig to the current process."

TheShermanTanker

Would've used the C++14 attribute syntax for this, but oh well

tstuefe · 2024-05-26T06:08:25Z

@kimbarrett @MBaesken

When this was written, the point was to raise a "real" SIGFPE. That matters because the behavior is subtly different from a real signal compared to one faked with raise (asynchronous vs synchronous).

Among other things, this SIGFPE is used for regression testing https://bugs.openjdk.org/browse/JDK-8065895.

JDK-8065895 described a situation where we accidentally blocked all but the currently processed signal in the signal handler. That meant if we process a synchronous signal (e.g. SIGSEGV) and another, different, synchronous signal happens (e.g. SIGILL), the VM won't handle it in the secondary handler. Instead, depending on the OS, the process either dies immediately without core or it hangs in the kernel.

To regression-test the fix, we need to be able to trigger two different synchronous signals. I believe I used SIGILL and SIGSEGV in my original patch in the closed-source SAP JVM. Both are easy to trigger. But then I got resistance against triggering SIGILL, though, and therefore OpenJDK triggers SIGFPE instead of SIGILL. With the unfortunate effect that the test won't work as expected on all platforms.

Apart from JDK-8065895, it was also used to check hs-err printing in general. But I guess for that we could use a raised signal.

Replacing the triggering with raise will make the regression test for JDK-8065895 toothless. We may just as well remove it then. I remember it being a pain to investigate (no core, no hs-err file), so we should come up with a replacement.

We could replace it with an explicit check that tests that the signal handler masks inside error reporting are set up correctly. That is not the same as the real thing, but I guess it would be the next best thing.

If we keep it, we need a comment in controlled_crash, because this discussion re-occurs at regular intervals.

dholmes-ora

As Thomas notes we intentionally want to test a synchronous signal if possible, so doing the minimum we can to "fix" ubsan is fine by me.

Thanks.

MBaesken · 2024-05-27T06:49:44Z

Would've used the C++14 attribute syntax for this, but oh well

You can put it on the list, why C++ 14 is desired (at some point in future sooner or later we will go anyway to some more current version of the standard).

MBaesken · 2024-05-27T07:06:01Z

I adjusted the comment a bit.

tstuefe

+1

MBaesken · 2024-05-27T10:26:32Z

Thanks for the reviews !

/integrate

openjdk · 2024-05-27T10:27:43Z

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

0e7ea39: 8332678: Serial: Remove use of should_clear_all_soft_refs in serial folder
72fbfe1: 8330577: G1 sometimes sends jdk.G1HeapRegionTypeChange for non-changes
0889155: 8332885: Clarify failure_handler self-tests
7f0ad51: 8332898: failure_handler: log directory of commands
617edf3: 8332497: javac prints an AssertionError when annotation processing runs on program with module imports
ffa4bad: 8332527: ZGC: generalize object cloning logic
a3a367e: 8332871: Parallel: Remove public bits APIs in ParMarkBitMap
61db2f5: 8079167: Fix documentation for G1SATBBufferEnqueueingThresholdPercent == 0
a083364: 8321292: SerialGC: NewSize vs InitialHeapSize check has an off-by-one error
16dba04: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null
... and 17 more: https://git.openjdk.org/jdk/compare/6a35311468222f9335b43d548df2ecb80746b389...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-27T10:27:48Z

@MBaesken Pushed as commit 1b8dea4.

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

kimbarrett · 2024-05-27T19:42:15Z

When this was written, the point was to raise a "real" SIGFPE. That matters because the behavior is subtly different from a real signal compared to one faked with raise (asynchronous vs synchronous).

Among other things, this SIGFPE is used for regression testing https://bugs.openjdk.org/browse/JDK-8065895.
[...]

Thanks for the background info. In light of that, I think the approach of using an attribute to suppress the
ubsan failure is good, and agree there needs to be some commentary added to explain what's going on.

JDK-8332894

c747e0c

openjdk bot changed the title ~~JDK-8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero~~ 8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero May 24, 2024

openjdk bot added the rfr Pull request is ready for review label May 24, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label May 24, 2024

kimbarrett reviewed May 25, 2024

View reviewed changes

TheShermanTanker reviewed May 26, 2024

View reviewed changes

dholmes-ora approved these changes May 27, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 27, 2024

adjust comment

b297f19

tstuefe approved these changes May 27, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 27, 2024

openjdk bot closed this May 27, 2024

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

8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero #19394

8332894: ubsan: vmError.cpp:2090:26: runtime error: division by zero #19394

Uh oh!

Conversation

MBaesken commented May 24, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Warning

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 24, 2024

Uh oh!

openjdk bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 24, 2024

Uh oh!

mlbridge bot commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

JohnTortugo commented May 24, 2024

Uh oh!

kimbarrett May 25, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken May 27, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 27, 2024

Choose a reason for hiding this comment

Uh oh!

MBaesken May 27, 2024

Choose a reason for hiding this comment

Uh oh!

dholmes-ora May 27, 2024

Choose a reason for hiding this comment

Uh oh!

kimbarrett May 27, 2024

Choose a reason for hiding this comment

Uh oh!

TheShermanTanker left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented May 26, 2024

Uh oh!

dholmes-ora left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MBaesken commented May 27, 2024

Uh oh!

MBaesken commented May 27, 2024

Uh oh!

tstuefe left a comment

Choose a reason for hiding this comment

Uh oh!

MBaesken commented May 27, 2024

Uh oh!

openjdk bot commented May 27, 2024

Uh oh!

openjdk bot commented May 27, 2024

Uh oh!

kimbarrett commented May 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

MBaesken commented May 24, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 24, 2024 •

edited

Loading

mlbridge bot commented May 24, 2024 •

edited

Loading

dholmes-ora left a comment •

edited

Loading