8335237: ubsan: vtableStubs.hpp is_vtable_stub exclude from ubsan checks

[MBaesken]

JDK-8331725 shows an ubsan (undefined behavior) issue in vtableStubs.hpp is_vtable_stub() .

/open_jdk/jdk_test/jdk/src/hotspot/share/code/vtableStubs.hpp:176:60: runtime error: load of value 204, which is not a valid value for type 'bool'
#0 0x110a6ad7e in VtableStubs::entry_point(unsigned char*) vtableStubs.cpp:280
#1 0x10f4cc8e6 in CompiledIC::is_megamorphic() const compiledIC.cpp:293
#2 0x10f4cc95d in CompiledIC::update(CallInfo*, Klass*) compiledIC.cpp:268
#3 0x110592eed in SharedRuntime::resolve_helper(bool, bool, JavaThread*) sharedRuntime.cpp:1366
#4 0x11058c0b3 in SharedRuntime::resolve_virtual_call_C(JavaThread*) sharedRuntime.cpp:1514
#5 0x12cd2e55a ()
#6 0x12580e03b ()
#7 0x12cc1f321 ()
#8 0x12cc1f321 ()

From the comments of the issue it seems while the coding could be improved, the problem is not very severe ('The reason bad bool values are seen is because there is no VtableStub object at that location. The reason this works is that we use the data at this location to generate a hash code, do a hash table lookup and then check the actual address for equality. Generating a bogus hash is harmless, ... ')
so as long as nothing better was found, we can exclude the method from ubsan checking.

---

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-8335237: ubsan: vtableStubs.hpp is_vtable_stub exclude from ubsan checks (Bug - P4)

Reviewers

• Martin Doerr (@TheRealMDoerr - Reviewer)
• Christoph Langer (@RealCLanger - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19925

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8335237: ubsan: vtableStubs.hpp  is_vtable_stub exclude from ubsan checks

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

• f4d8c00: 8334562: Automate com/sun/security/auth/callback/TextCallbackHandler/Default.java test
• 49eb00d: 8299813: java/nio/channels/DatagramChannel/Disconnect.java fails with jtreg test timeout due to lost datagram
• 8ec378a: 8277949: (dc) java/nio/channels/DatagramChannel/AdaptorBasic.java failed in timeout
• c798316: 8269657: Test java/nio/channels/DatagramChannel/Loopback.java failed: Unexpected message
• 99d2bbf: 8334433: jshell.exe runs an executable test.exe on startup
• 6f4ddc2: 8335142: compiler/c1/TestTraceLinearScanLevel.java occasionally times out with -Xcomp
• 3b3a19e: 8335314: Problem list compiler/uncommontrap/DeoptReallocFailure.java
• d457609: 8319947: Recursive lightweight locking: s390x implementation
• c47a0e0: 8334147: Shenandoah: Avoid taking lock for disabled free set logging
• 308a812: 8334645: Un-problemlist vmTestbase/nsk/sysdict/vm/stress/chain/chain007/chain007.java
• ... and 15 more: https://git.openjdk.org/jdk/compare/37e7698c29b8673b904945d397f0698ccd16d27b...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]

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

• hotspot-compiler

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 (dc4fa308)
• 00: Full (3538a479)

[TheRealMDoerr]

I'd like to have a comment explaining it if we decide to do it this way.

[MBaesken]

I'd like to have a comment explaining it if we decide to do it this way.

Should I just point to the other bug JDK-8331725 in the comment?
Or any good suggestions ?

[TheRealMDoerr]

Maybe:
We reinterpret arbitrary memory as VtableStub. This does not cause failures because the lookup/equality check will reject false objects. Disabling UBSan is a temporary workaround until JDK-8331725 is fixed.

[MBaesken]

Thanks, I added a comment following your suggestions.

[xmas92]

It is only harmless from the point of view that we cannot regardless of hash generated find an entry which have its entrypoint at the location we are checking if it is not a vtable_stub.

However we have no guarantees what the compiler might do in the presence of UB. So far it has not been a problem.

JDK-8331725 has not been a priority for me. But I can get my fix out in a couple of days. The issue with fixing this is that C++ does not define an object representation for false. So either it has to rely on implementation specific behaviour (that false is all 0 bits in memory) or we have to create a boolean like enum class which is used and specifies the actual values for true and false.

I suspect we have assumptions elsewhere in the code base that false is represented by 0x0. It is a lot easier to work with Java's jni boolean in this regard.

[MBaesken]

Thanks for the reviews !

/integrate

[openjdk]

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

• f4d8c00: 8334562: Automate com/sun/security/auth/callback/TextCallbackHandler/Default.java test
• 49eb00d: 8299813: java/nio/channels/DatagramChannel/Disconnect.java fails with jtreg test timeout due to lost datagram
• 8ec378a: 8277949: (dc) java/nio/channels/DatagramChannel/AdaptorBasic.java failed in timeout
• c798316: 8269657: Test java/nio/channels/DatagramChannel/Loopback.java failed: Unexpected message
• 99d2bbf: 8334433: jshell.exe runs an executable test.exe on startup
• 6f4ddc2: 8335142: compiler/c1/TestTraceLinearScanLevel.java occasionally times out with -Xcomp
• 3b3a19e: 8335314: Problem list compiler/uncommontrap/DeoptReallocFailure.java
• d457609: 8319947: Recursive lightweight locking: s390x implementation
• c47a0e0: 8334147: Shenandoah: Avoid taking lock for disabled free set logging
• 308a812: 8334645: Un-problemlist vmTestbase/nsk/sysdict/vm/stress/chain/chain007/chain007.java
• ... and 15 more: https://git.openjdk.org/jdk/compare/37e7698c29b8673b904945d397f0698ccd16d27b...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 486aa11.

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