8287982: Concurrent implicit attach from native threads crashes VM

[AlanBateman]

If several native threads attach to the VM at around the same time, and before any threads get an automatically generated name are created, then the VM may crash attempting to access the thread status. The issue exists for native threads that attach explicitly with JNI AttachCurrentThread without a thread name, or native threads that attach implicitly by using a function pointer to do an up call.

The issue that raises its head periodically is that native threads that JNI attach do the initializaiton of the Thread object in the context of the attaching thread. Great care must be taken because Java code is executing in the context of a Thread that is not fully initialized. The right thing is probably to create the Thread object in another thread, using the service thread has been mentioned. The issue at this time arises when two or more native threads attempt to attach without thread names at around the same time. The first thread that needs an automatically generated name triggers the loading and initialization of a helper class. If there are other threads attaching at the same time then they may have to wait on the monitor which can trigger the crash because the field holder with the thread status is not created at this time. Crashes in monitor enter and notify have been observed. Coleen has changed this code so that linking and initialization uses a mutex (JDK-8288064) so this specific crash doesn't duplicate in the main line. The short term fix for openjdk/jdk19 is to reorder the initialization so that field holder with the thread status is created before setting the name.

Creating a jtreg test with the conditions to duplicate this issue is complicated. The jtreg main wrapper creates the main thread with an automatically generated thread name before it runs the test main method. This is the reason that the test needs to launch a new VM with the right setup to exercise both explicit and implicit attach.

---

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-8287982: Concurrent implicit attach from native threads crashes VM

Reviewers

• David Holmes (@dholmes-ora - Reviewer)
• Robbin Ehn (@robehn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/28.diff

[bridgekeeper]

👋 Welcome back alanb! 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]

@AlanBateman The following labels will be automatically applied to this pull request:

• build
• core-libs

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.

[AlanBateman]

/label add hotspot-runtime
/label remove build

[openjdk]

@AlanBateman
The hotspot-runtime label was successfully added.

[openjdk]

@AlanBateman
The build label was successfully removed.

[mlbridge]

Webrevs

• 00: Full (fee844ec)

[dholmes-ora]

Looks good.

And I confirmed that the VM code will correctly handle a null name from the current Java thread.

Thanks.

[openjdk]

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

8287982: Concurrent implicit attach from native threads crashes VM

Reviewed-by: dholmes, rehn

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

• b9c3966: 8288671: Problematic fix for font boosting
• 453e8be: 8288527: broken link in java.base/java/util/zip/package-summary.html
• 33d0363: 8288741: JFR: Change package name of snippet files
• 0408f9c: 8288663: JFR: Disabling the JfrThreadSampler commits only a partially disabled state
• 1cf83a4: 8287800: JFR: Incorrect error message when starting recording with missing .jfc file
• 09da87c: 8288485: jni/nullCaller/NullCallerTest.java failing (ppc64)
• ed714af: 8288564: C2: LShiftLNode::Ideal produces wrong result after JDK-8278114
• ae030bc: 8288397: AArch64: Fix register issues in SVE backend match rules
• f12d044: 8288692: jdk/javadoc/doclet/testTagMisuse/TestTagMisuse.java fails after JDK-8288545
• 97544be: 8268398: 15% increase in JFR footprint in Noop-Base
• ... and 1 more: https://git.openjdk.org/jdk19/compare/53bf1bfdabb79b37afedd09051d057f9eea620f2...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.

[AlanBateman]

And I confirmed that the VM code will correctly handle a null name from the current Java thread.

Thanks, I checked that too.

One thing that I'm wondering for the test is whether to move it to test/hotspot/jtreg/runtime/jni/ so it lives with the other tests for JNI. I had expected to find other tests for AttachCurrentThread in that location but we don't.

[robehn]

Looks good.

[dholmes-ora]

One thing that I'm wondering for the test is whether to move it to test/hotspot/jtreg/runtime/jni/ so it lives with the other tests for JNI. I had expected to find other tests for AttachCurrentThread in that location but we don't.

As this doesn't change hotspot code it seems more appropriate to have the regression test where it is. We have a number of tests that use AttachCurrentThread but very few actual regression tests for it.

Thanks.

[AlanBateman]

/integrate

[openjdk]

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

• 9e2d9ac: 8288529: broken link in java.xml
• 70008da: 8287971: Throw exception for missing values in .jpackage.xml
• d7b43af: 8288761: SegmentAllocator:allocate(long bytesSize) not throwing IAEx when bytesSize < 0
• 834d92d: 8288754: GCC 12 fails to build zReferenceProcessor.cpp
• 198cec9: 8286103: VThreadMonitorTest fails "assert(!current->cont_fastpath() || (current->cont_fastpath_thread_state() && !interpreted_native_or_deoptimized_on_stack(current))) failed"
• 97200a7: 8278053: serviceability/jvmti/vthread/ContStackDepthTest/ContStackDepthTest.java failing in loom repo with Xcomp
• 31d981e: 8288532: additional review changes for JDK-8286830
• a144988: 8288139: JavaThread touches oop after GC barrier is detached
• e26d3b3: 8288497: add support for JavaThread::is_oop_safe()
• c74a923: 8288531: Empty spans in mobile navigation markup
• ... and 12 more: https://git.openjdk.org/jdk19/compare/53bf1bfdabb79b37afedd09051d057f9eea620f2...master

Your commit was automatically rebased without conflicts.

[openjdk]

@AlanBateman Pushed as commit 7cf71bc.

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