8280422: thread_from_jni_environment can never return NULL #7193

dholmes-ora · 2022-01-23T23:20:38Z

Analysing the code and its use (see JBS issue for gory details) we can see that thread_from_jni_environment can never actually return NULL, so we change it to not appear to do so and thus keep static analysis tools happy. We also always validate the incoming JNIEnv for debug builds and scrap the unused VerifyJNIEnvThread flag.

Testing: tiers 1-5

Thanks,
David

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8280422: thread_from_jni_environment can never return NULL

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)
Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7193/head:pull/7193
$ git checkout pull/7193

Update a local copy of the PR:
$ git checkout pull/7193
$ git pull https://git.openjdk.java.net/jdk pull/7193/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7193

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7193.diff

bridgekeeper · 2022-01-23T23:21:20Z

👋 Welcome back dholmes! 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 · 2022-01-23T23:23:29Z

@dholmes-ora The following label will be automatically applied to this pull request:

hotspot-runtime

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 · 2022-01-23T23:27:21Z

Webrevs

01: Full - Incremental (a8501509)
00: Full (f47198c7)

src/hotspot/share/runtime/thread.hpp

shipilev

Looks sensible to me.

openjdk · 2022-01-24T09:38:19Z

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

8280422: thread_from_jni_environment can never return NULL

Reviewed-by: shade, kbarrett

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

e307655: 8280403: RegEx: String.split can fail with NPE in Pattern.CharPredicate::match
52ddbe2: Merge
ead9fec: 8280441: Missing "classpath exception" in several files from jdk.httpserver
7d2ef9d: 8279179: Update nroff pages in JDK 18 before RC
8e82d00: 8280492: Use cross-module syntax for cross-module links
a541666: 8280474: Garbage value passed to getLocaleInfoWrapper in HostLocaleProviderAdapter_md
a825a4a: 8279861: Clarify 'rect' parameters and description of paintTabBorder method in BasicTabbedPaneUI
0b5c54b: 8279798: Javadoc for BasicTabbedPaneUI is inconsistent
acd9829: 8279794: Fix typos in BasicScrollBarUI: Laysouts a vertical scroll bar
dae2226: 8279795: Fix typo in BasicFileChooserUI: Constucts -> Constructs
... and 23 more: https://git.openjdk.java.net/jdk/compare/35ee0f38c6d5e35fb50a059a189e032df7f7e7c5...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.

dholmes-ora · 2022-01-24T22:59:43Z

Thanks for the review @shipilev !

kimbarrett

Looks good.

kimbarrett · 2022-01-25T00:38:06Z

src/hotspot/share/runtime/thread.hpp

-      return NULL;
-    } else {
-      return thread_from_jni_env;
+    JavaThread* current = (JavaThread*)((intptr_t)env - in_bytes(jni_environment_offset()));


[pre-existing] If I were writing this I would have used char* rather than intptr_t.

dholmes-ora · 2022-01-25T01:21:25Z

Thanks @kimbarrett !

/integrate

openjdk · 2022-01-25T01:22:57Z

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

e307655: 8280403: RegEx: String.split can fail with NPE in Pattern.CharPredicate::match
52ddbe2: Merge
ead9fec: 8280441: Missing "classpath exception" in several files from jdk.httpserver
7d2ef9d: 8279179: Update nroff pages in JDK 18 before RC
8e82d00: 8280492: Use cross-module syntax for cross-module links
a541666: 8280474: Garbage value passed to getLocaleInfoWrapper in HostLocaleProviderAdapter_md
a825a4a: 8279861: Clarify 'rect' parameters and description of paintTabBorder method in BasicTabbedPaneUI
0b5c54b: 8279798: Javadoc for BasicTabbedPaneUI is inconsistent
acd9829: 8279794: Fix typos in BasicScrollBarUI: Laysouts a vertical scroll bar
dae2226: 8279795: Fix typo in BasicFileChooserUI: Constucts -> Constructs
... and 23 more: https://git.openjdk.java.net/jdk/compare/35ee0f38c6d5e35fb50a059a189e032df7f7e7c5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-25T01:23:12Z

@dholmes-ora Pushed as commit f35df5b.

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

parttimenerd · 2022-02-21T12:22:57Z

This seems to break AsyncGetCallTrace in some rare cases, as "thread_from_jni_environment can never actually return NULL" does not always hold.

I run into this rather often when running a profiler on the dacapo benchmark suite with an debug build of the JDK.

parttimenerd · 2022-02-21T12:29:34Z

I would suggest to add a method for checking the termination status in cases where it might happen:

  // Returns whether current thread as indicated by the given JNIEnv
  // is terminated.
  // We don't assert it is Thread::current here as that is done at the
  // external JNI entry points where the JNIEnv is passed into the VM.
  static bool is_thread_from_jni_environment_terminated(JNIEnv* env) {
    JavaThread* current = (JavaThread*)((intptr_t)env - in_bytes(jni_environment_offset()));
    // We can't get here in a thread that has completed its execution and so
    // "is_terminated", but a thread is also considered terminated if the VM
    // has exited, so we have to check this and block in case this is a daemon
    // thread returning to the VM (the JNI DirectBuffer entry points rely on
    // this).
    return current->is_terminated();
  }

dholmes-ora · 2022-02-21T12:40:49Z

@parttimenerd 👍

This seems to break AsyncGetCallTrace in some rare cases, as "thread_from_jni_environment can never actually return NULL" does not always hold.

Can you please provide further information. As a correctly used JNIEnv can never produce a NULL thread as the JNIEnv is a sub-object of the JavaThread instance.

parttimenerd · 2022-02-21T12:46:02Z

AsyncGetCallTrace is implemented in forte.cpp:

void AsyncGetCallTrace(ASGCT_CallTrace *trace, jint depth, void* ucontext) {
  JavaThread* thread;

  if (trace->env_id == NULL ||
    (thread = JavaThread::thread_from_jni_environment(trace->env_id)) == NULL ||
    thread->is_exiting()) {

    // bad env_id, thread has exited or thread is exiting
    trace->num_frames = ticks_thread_exit; // -8
    return;
  }

I'm going to create a separate issue with a PR for the discussion.

dholmes-ora · 2022-02-21T13:03:57Z

That condition should just be:

if (trace->env_id == NULL || (thread = JavaThread::thread_from_jni_environment(trace->env_id))->is_exiting()) {

parttimenerd · 2022-02-21T13:07:11Z

That does not address the issue, as JavaThread::thread_from_jni_environment(trace->env_id) never returns in case is_terminating() returns true.

dholmes-ora · 2022-02-21T13:18:13Z

Correct but this PR did not introduce that behaviour. I only changed the name of the local variable and added the comment.

parttimenerd · 2022-02-21T13:21:21Z

src/hotspot/share/runtime/thread.hpp

+    // this).
+    if (current->is_terminated()) {
+      current->block_if_vm_exited();
+      ShouldNotReachHere();


This breaks AsyncGetCallTrace

If we reach that code then the VM aborts. It should be impossible to reach that statement and thus impossible to actually return NULL in the old code. Please elaborate under what conditions you think this code can be reached.

AsyncProfiler on a debug build of OpenJDK profiling the dacapo tomcat benchmark.

parttimenerd · 2022-02-21T13:22:41Z

It seems to me that this commit introduces the ShouldNotReachHere() when before NULL would be returned.

parttimenerd · 2022-02-21T14:47:07Z

I've created a PR with a fix at #7559

8280422: thread_from_jni_environment can never return NULL

f47198c

openjdk bot added the rfr Pull request is ready for review label Jan 23, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jan 23, 2022

DamonFool reviewed Jan 23, 2022

View reviewed changes

src/hotspot/share/runtime/thread.hpp Show resolved Hide resolved

Update copyright

a850150

shipilev approved these changes Jan 24, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 24, 2022

kimbarrett approved these changes Jan 25, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 25, 2022

openjdk bot closed this Jan 25, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2022

dholmes-ora deleted the 8280422-thread-from-jni branch January 25, 2022 02:18

parttimenerd reviewed Feb 21, 2022

View reviewed changes

parttimenerd mentioned this pull request Feb 21, 2022

8282200: ShouldNotReachHere() reached by AsyncGetCallTrace after JDK-8280422 #7559

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8280422: thread_from_jni_environment can never return NULL #7193

8280422: thread_from_jni_environment can never return NULL #7193

dholmes-ora commented Jan 23, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 23, 2022

openjdk bot commented Jan 23, 2022

mlbridge bot commented Jan 23, 2022 •

edited

Loading

shipilev left a comment

openjdk bot commented Jan 24, 2022 •

edited

Loading

dholmes-ora commented Jan 24, 2022

kimbarrett left a comment

kimbarrett Jan 25, 2022

dholmes-ora commented Jan 25, 2022

openjdk bot commented Jan 25, 2022

openjdk bot commented Jan 25, 2022

parttimenerd commented Feb 21, 2022 •

edited

Loading

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022 •

edited

Loading

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022

parttimenerd Feb 21, 2022

dholmes-ora Feb 21, 2022

parttimenerd Feb 21, 2022

parttimenerd commented Feb 21, 2022

parttimenerd commented Feb 21, 2022

8280422: thread_from_jni_environment can never return NULL #7193

8280422: thread_from_jni_environment can never return NULL #7193

Conversation

dholmes-ora commented Jan 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 23, 2022

openjdk bot commented Jan 23, 2022

mlbridge bot commented Jan 23, 2022 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 24, 2022 • edited Loading

dholmes-ora commented Jan 24, 2022

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett Jan 25, 2022

Choose a reason for hiding this comment

dholmes-ora commented Jan 25, 2022

openjdk bot commented Jan 25, 2022

openjdk bot commented Jan 25, 2022

parttimenerd commented Feb 21, 2022 • edited Loading

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022 • edited Loading

parttimenerd commented Feb 21, 2022

dholmes-ora commented Feb 21, 2022

parttimenerd Feb 21, 2022

Choose a reason for hiding this comment

dholmes-ora Feb 21, 2022

Choose a reason for hiding this comment

parttimenerd Feb 21, 2022

Choose a reason for hiding this comment

parttimenerd commented Feb 21, 2022

parttimenerd commented Feb 21, 2022

dholmes-ora commented Jan 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 23, 2022 •

edited

Loading

openjdk bot commented Jan 24, 2022 •

edited

Loading

parttimenerd commented Feb 21, 2022 •

edited

Loading

dholmes-ora commented Feb 21, 2022 •

edited

Loading