8331725: ubsan: pc may not always be the entry point for a VtableStub

[xmas92]

VtableStub* VtableStubs::entry_point(address pc) has multiple issues causing undefined behaviour. Most of it stems from the fact that the pc sent in may sometimes not be the entry point of a VtableStub. We check this by doing a lookup in a hash table, but to generate the hash we need to extract data from the VtableStub. The original implementation simply cast the address to a *VtableStub and called the member function getters. This caused both unaligned accesses and bad bool values, all symptoms of the fact that we reinterpret_cast the memory as a VtableStub.

This change will instead look at the raw bytes when generating the hash which neither suffers from unaligned access nor bad reinterpret_casts.

My initial patch did not create an enum class to represent the boolean value but instead looked at the object representation of the bool field. However this suffers from the fact that the object representation of false is implementation defined. I assumed it to be all 0 bits. It was also required unfortunate extra logic to handle the cases when sizeof(bool) != 1. Alt patch: https://github.com/openjdk/jdk/commit/b20140f49983b1903f9038e4e47def792c4493ac

There may be some better name than unsafe_hash. Suggestions?

Verified that this patch resolves the UB. Running testing.

Side note: There is still an unaligned access issue on aarch64 because of the way we construct VtableStub. We align the chunk where we create the VtableStub so that the <VtableStub address> + sizeof(VtableStub) is aligned to the platforms code entry alignment. On all platforms but aarch64 <VtableStub address> + sizeof(VtableStub) % pd_code_alignment == 0 implies <VtableStub address> % alignof(VtableStub) == 0. https://github.com/xmas92/jdk/blob/beea5305b071820e2b128a55c5ca384caf470fdd/src/hotspot/share/code/vtableStubs.hpp#L168-L171

Also note that there are already assumptions in the JVM about the object representation of bool. On x86 MacroAssembler::testbool assumes that false is all 0 bits, and in addition to that MacroAssembler::movbool assumes that 1 is true.

---

Progress

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

Issue

• JDK-8331725: ubsan: pc may not always be the entry point for a VtableStub (Bug - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19968

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back aboldtch! 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[bot]]

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

8331725: ubsan: pc may not always be the entry point for a VtableStub

Reviewed-by: kvn, mbaesken

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

• 0e0dfca21f64ecfcb3e5ed7cdc2a173834faa509: 8330806: test/hotspot/jtreg/compiler/c1/TestLargeMonitorOffset.java fails on ARM32
• f3ff4f7427c3c3f5cb2a115a61462bb9d28de1cd: 8335882: platform/cgroup/TestSystemSettings.java fails on Alpine Linux
• 8f62f31dff564289a2422d58e8ecd5062d443b81: 8335906: [s390x] Test Failure: GTestWrapper.java
• 2a2964759c73b3b9ab6afaad109383c89952977b: 8334777: Test javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java failed with NullPointerException
• 564a72e1dba0f145600c8e7eff66992fbf294df0: 8335955: JDK-8335742 wrongly used a "JDK-" prefix in the problemlist bug number
• 9c7a6eabb93c570fdb74076edc931576ed6be3e0: 8312125: Refactor CDS enum class handling
• bb1f8a1698553d5962569ac8912edd0d7ef010dd: 8335904: Fix invalid comment in ShenandoahLock
• babf6df7d97e4beedb25e689634d999412c1e950: 8334757: AssertionError: Missing type variable in where clause
• 3733fe3a207078b585421cd2a098e808fafaa817: 8335789: [TESTBUG] XparColor.java test fails with Error. Parse Exception: Invalid or unrecognized bugid: @
• 3a87eb5c4606ce39970962895315567e8606eba7: 8335126: Shenandoah: Improve OOM handling
• ... and 86 more: https://git.openjdk.org/jdk/compare/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d...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[bot]]

@xmas92 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[bot]]

Webrevs

• 00: Full (6629f475)

[xmas92]

Thanks for the reviews. Tested this for tier1 through tier7. Also ran some sanity benchmarks, looks neutral. /integrate

[openjdk[bot]]

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

• fb9a227e02ebf826edb762283e15dd7e402f8433: 8313909: [JVMCI] assert(cp->tag_at(index).is_unresolved_klass()) in lookupKlassInPool
• e6c5aa7a6cb54c647d261facdcffa6a410849627: 8336012: Fix usages of jtreg-reserved properties
• e0fb949460d0c7e2ab1697a6466e7d4831a20a33: 8335779: JFR: Hide sleep events
• 537d20afbff255489a7b1bdb0410b9d1aba715b7: 8335766: Switch case with pattern matching and guard clause compiles inconsistently
• a44b60c8c14ad998e51239f48e64779304aaac50: 8335778: runtime/ClassInitErrors/TestStackOverflowDuringInit.java fails on ppc64 platforms after JDK-8334545
• b5909cabeef22954f4d9c642b1cbf288b3454562: 8323242: Remove vestigial DONT_USE_REGISTER_DEFINES
• dcf4e0d51f392afe2711223484e932e3826e8864: 8335966: Remove incorrect problem listing of java/lang/instrument/NativeMethodPrefixAgent.java in ProblemList-Virtual.txt
• 1472124489c841642996ae984e21c533ffec8091: 8333364: Minor cleanup could be done in com.sun.crypto.provider
• 7e11fb702696df733ca89d325200f2e9414402d9: 8335688: Fix -Wzero-as-null-pointer-constant warnings from fflush calls in jvmti tests
• 531a6d85b00b88688668ab1ced0db6ce0214a5f1: 8335911: Document ccls indexer in doc/ide.md
• ... and 96 more: https://git.openjdk.org/jdk/compare/d9bcf061450ebfb7fe02b5a50c855db1d9178e5d...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@xmas92 Pushed as commit fb66716a1bc914db194c5b0b833cc2317704f166.

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