8322511: [11u] JfrCheckpointThreadClosure::do_thread crashes when fetching thread_id

[jia-wei-tang]

Threads which are in the state of is_attaching_via_jni may have the NULL object as their _threadObj .
So the JVM will crash when calling JfrCheckpointThreadClosure::do_thread().

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• JDK-8322511 needs maintainer approval
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8322511: [11u] JfrCheckpointThreadClosure::do_thread crashes when fetching thread_id (Bug - P4 - Approved)

Reviewers

• Paul Hohensee (@phohensee - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2413/head:pull/2413
$ git checkout pull/2413

Update a local copy of the PR:
$ git checkout pull/2413
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2413/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2413

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2413.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @jia-wei-tang, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jia-wei-tang" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

⚠️ @jia-wei-tang a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master 3212030bdbbb4f2aae35bc8db5ab90869d232a9a
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

[jia-wei-tang]

/covered

[bridgekeeper]

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[GoeLin]

Hi,
is this an error in 11 only?
If so, explain why & where this was fixed in higher versions.
Else please close this PR, reopen it against repo jdk. Once it is submitted there, backport it to 21, 17 and 11.
See also https://wiki.openjdk.org/display/JDKUpdates/How+to+contribute+or+backport+a+fix

[jia-wei-tang]

@GoeLin This is an error in 17,11 and 8.

1. In the newest JDK, commit JDK-8276125 has solved half of the problem. It prevents jvm from visiting the null object when getting thread_group_id. And this commit is a bug fix.
2. In 17, commit JDK-8226511 has solved the other part of the problem. It prevents jvm from visiting the null object when getting thread_id. But this commit is a Sub-task, and I speculate that developers didn't notice that this place would cause a jvm crash at that time.
   Thus, in 17, I need to port JDK-8276125. In 11 and 8, I need to combine both commits I mentioned above to fix this error.
   Since the newest JDK doesn't have this problem, I am wondering where to open this PR.

[jia-wei-tang]

/signed

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge]

Webrevs

• 04: Full - Incremental (d9a69c9a)
• 03: Full - Incremental (0c58a66f)
• 02: Full - Incremental (0cc754ad)
• 01: Full - Incremental (1244115a)
• 00: Full (8e7be57f)

[jia-wei-tang]

@GoeLin Hi, I has not been an author so I cannot use /backport to backport JDK-8276125 from jdk to jdk17u-dev. I asked my colleague to do this for me. I read the description of JDK-8276125, I think the issue (JDK-8322511) is different from that even though they are similar. Besides, JDK-8276125 isn't a clean backport to jdk11.

[openjdk]

@jia-wei-tang Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[openjdk]

@jia-wei-tang Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[jia-wei-tang]

@GoeLin Hi, I made some changes to fix this bug. Combine this PR and BackPort 8276125 can fix the JVM crash.

Some details about how JVM visit the null object _threadObj as follows.

1. JNI call AttachCurrentThread
2. JVM creates a JavaThread, which is set in the state of is_attaching_via_jni
3. Threads::add add this new JavaThread into ThreadList
4. The JavaThread continues to initialize and call allocate_threadObj to fill in _threadObj

However, between 3-4 steps, a JFR safepoint operation is executing. It traverses the ThreadList and attempts to read the id of each thread. The id is gotten by using _threadObj as a base value and adding a offset. Thus, the JVM crash happens when read the illegal address.

[bridgekeeper]

@jia-wei-tang This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[jia-wei-tang]

@phohensee @jerboaa Could you pls approve this JDK-8322511 and review my codes? I showed the details about how JVM visit the null object _threadObj before and the change is harmless (only add the null condition check).

[openjdk]

⚠️ @jia-wei-tang This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

[jia-wei-tang]

/approval request Fix JVM crash by adding a null condition check

[openjdk]

@jia-wei-tang
8322511: The approval request has been created successfully.

[jia-wei-tang]

The results of related JFR testcases are all successful.

TEST TOTAL PASS FAIL ERROR
jtreg:test/jdk/jdk/jfr 434 434 0 0

[GoeLin]

@jia-wei-tang, can you please add [11u] to the PR title as I did for the JBS issue? Thanks!

[jerboaa]

Please also enable GHA testing on your fork and trigger test runs!

[jia-wei-tang]

@GoeLin I changed the title of the pr and triggered GHA tests. May you approve JDK-8322511 ? Thanks a lot!

[openjdk]

@jia-wei-tang This change now passes all automated pre-integration checks.

After integration, the commit message for the final commit will be:

8322511: [11u] JfrCheckpointThreadClosure::do_thread crashes when fetching thread_id

Reviewed-by: phh

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 1 new commit pushed to the master branch:

• 7b7fbdf: 8267938: (sctp) SCTP channel factory methods should check platform support

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@phohensee) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[jia-wei-tang]

/integrate

[openjdk]

@jia-wei-tang
Your change (at version d9a69c9) is now ready to be sponsored by a Committer.

[phohensee]

/sponsor

[openjdk]

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

• c8ca55b: 8314220: Configurable InlineCacheBuffer size
• e6cec8f: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range
• eb5ef84: 8315677: Open source few swing JFileChooser and other tests
• cf9aa9b: 8306941: Open source several datatransfer and dnd AWT tests
• 9b40856: 8317287: [macos14] InterJVMGetDropSuccessTest.java: Child VM: abnormal termination
• fbac37b: 8307083: Open source some drag and drop tests 3
• a26445d: 8261404: Class.getReflectionFactory() is not thread-safe
• 7b7fbdf: 8267938: (sctp) SCTP channel factory methods should check platform support

Your commit was automatically rebased without conflicts.

[openjdk]

@phohensee @jia-wei-tang Pushed as commit 29f300e.

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