8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage

[MBaesken]

When running with ubsan - enabled binaries (--enable-ubsan),
in the vmTestbase/nsk/jdi tests some cases of memset on nullptr destinations are detected in get_object_monitor_usage .

// null out memory for robustness
memset(ret.waiters, 0, ret.waiter_count * sizeof(jthread *));
memset(ret.notify_waiters, 0, ret.notify_waiter_count * sizeof(jthread *));

probably we should add checks there.
Example :
vmTestbase/nsk/jdi/ObjectReference/entryCount/entrycount002/TestDescription.jtr

debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1560:11: runtime error: null pointer passed as argument 1, which is declared to never be null
debugee.stderr> #0 0x7ffb2568559c in JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1560
debugee.stderr> #1 0x7ffb27987bd7 in VM_GetObjectMonitorUsage::doit() src/hotspot/share/prims/jvmtiEnvBase.hpp:594
debugee.stderr> #2 0x7ffb28ddc2dd in VM_Operation::evaluate() src/hotspot/share/runtime/vmOperations.cpp:75
debugee.stderr> #3 0x7ffb28deac41 in VMThread::evaluate_operation(VM_Operation*) src/hotspot/share/runtime/vmThread.cpp:283
debugee.stderr> #4 0x7ffb28decc4f in VMThread::inner_execute(VM_Operation*) src/hotspot/share/runtime/vmThread.cpp:427
debugee.stderr> #5 0x7ffb28ded7b9 in VMThread::loop() src/hotspot/share/runtime/vmThread.cpp:493
debugee.stderr> #6 0x7ffb28ded8a7 in VMThread::run() src/hotspot/share/runtime/vmThread.cpp:177
debugee.stderr> #7 0x7ffb28b7e31a in Thread::call_run() src/hotspot/share/runtime/thread.cpp:225
debugee.stderr> #8 0x7ffb281c4971 in thread_native_entry src/hotspot/os/linux/os_linux.cpp:846
debugee.stderr> #9 0x7ffb2df416e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73)
debugee.stderr> #10 0x7ffb2d51550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: f732026552f6adff988b338e92d466bc81a01c37)

vmTestbase/nsk/jdi/ObjectReference/owningThread/owningthread002/TestDescription.jtr

debugee.stderr> /src/hotspot/share/prims/jvmtiEnvBase.cpp:1561:11: runtime error: null pointer passed as argument 1, which is declared to never be null
debugee.stderr> #0 0x7f1e070855bb in JvmtiEnvBase::get_object_monitor_usage(JavaThread*, _jobject*, jvmtiMonitorUsage*) src/hotspot/share/prims/jvmtiEnvBase.cpp:1561
debugee.stderr> #1 0x7f1e09387bd7 in VM_GetObjectMonitorUsage::doit() src/hotspot/share/prims/jvmtiEnvBase.hpp:594
debugee.stderr> #2 0x7f1e0a7dc2dd in VM_Operation::evaluate() src/hotspot/share/runtime/vmOperations.cpp:75
debugee.stderr> #3 0x7f1e0a7eac41 in VMThread::evaluate_operation(VM_Operation*) src/hotspot/share/runtime/vmThread.cpp:283
debugee.stderr> #4 0x7f1e0a7ecc4f in VMThread::inner_execute(VM_Operation*) src/hotspot/share/runtime/vmThread.cpp:427
debugee.stderr> #5 0x7f1e0a7ed7b9 in VMThread::loop() src/hotspot/share/runtime/vmThread.cpp:493
debugee.stderr> #6 0x7f1e0a7ed8a7 in VMThread::run() src/hotspot/share/runtime/vmThread.cpp:177
debugee.stderr> #7 0x7f1e0a57e31a in Thread::call_run() src/hotspot/share/runtime/thread.cpp:225
debugee.stderr> #8 0x7f1e09bc4971 in thread_native_entry src/hotspot/os/linux/os_linux.cpp:846
debugee.stderr> #9 0x7f1e0f9bf6e9 in start_thread (/lib64/libpthread.so.0+0xa6e9) (BuildId: 2f8d3c2d0f4d7888c2598d2ff6356537f5708a73)
debugee.stderr> #10 0x7f1e0ef1550e in clone (/lib64/libc.so.6+0x11850e) (BuildId: f732026552f6adff988b338e92d466bc81a01c37)

---

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 8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage

Issue

• JDK-8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage (Bug - P4)

Reviewers

• Serguei Spitsyn (@sspitsyn - Reviewer)
• Martin Doerr (@TheRealMDoerr - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19450

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19450.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:

8333149: ubsan : memset on nullptr target detected in jvmtiEnvBase.cpp get_object_monitor_usage

Reviewed-by: sspitsyn, mdoerr

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

• 6cda4c5: 8321543: Update NSS to version 3.96
• c003c12: 8331865: Consolidate size and alignment checks in LayoutPath
• 6d718ae: 8324341: Remove redundant preprocessor #if's checks
• 9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 labels will be automatically applied to this pull request:

• hotspot
• serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (bda06369)

[sspitsyn]

Looks good. Thank you for taking care about it.

[TheRealMDoerr]

LGTM.

[MBaesken]

Hi Martin and Serguei, thanks for the reviews !

/integrate

[openjdk]

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

• fed2b56: 8320999: RISC-V: C2 RotateLeftV
• 6cda4c5: 8321543: Update NSS to version 3.96
• c003c12: 8331865: Consolidate size and alignment checks in LayoutPath
• 6d718ae: 8324341: Remove redundant preprocessor #if's checks
• 9b64ece: 8332904: ubsan ppc64le: c1_LIRGenerator_ppc.cpp:581:21: runtime error: signed integer overflow: 9223372036854775807 + 1 cannot be represented in type 'long int'

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 43a2f17.

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

[dholmes-ora]

@MBaesken This was not proposed as a trivial PR and so is subject to the 24 hour rule. Please don't push these ubsan "fixes" quickly as we need time to assess their validity and the right way to address them.

This fix looks wrong to me because those values cannot be null as it implies the allocate function failed which means we would not reach this code!

[TheRealMDoerr]

Please note that allocate sets *mem_ptr to nullptr if the size is 0. This is not an error.

[tstuefe]

I agree with David on the 24hr thing. We want others to stick to that rule, then we should keep the rule ourselves. The rule takes the pressure out of monitoring the patch flow.

But @TheRealMDoerr is right, the only logical way we can see a nullptr here is if there are no waiters/notifiers. A better solution may have been to move the memsets into their respective count > 0 conditions.

[dholmes-ora]

Okay, sorry, the zero case handling here is a little awkward. But it seems to me that if the counts are zero it is expected that memset does nothing, so the value of the pointer passed in is irrelevant. Shame it doesn't specify that. We should skip allocation and the subsequent memset when the count is zero.