8074860: Structured Exception Catcher missing around CreateJavaVM on Windows

[fthevenet]

Hi,

This is a backport of JDK-8082592 which adds Structured Exception Handling (SEH) to guard 'JNI_CreateJavaVM' on Windows.

The motivation for this backport is to help reduce the number of occurrences where a VM might terminate immediately without writing an error log Windows, making root cause analysis very difficult in such cases.

In addition, I plan to also backport JDK-8186199 as a follow-up, which does something similar to guard 'JNI_DestroyJavaVM'.

Thanks!

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)
• JDK-8074860 needs maintainer approval

Issue

• JDK-8074860: Structured Exception Catcher missing around CreateJavaVM on Windows (Bug - P4 - Approved)

Reviewers

• Andrew John Hughes (@gnu-andrew - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 449

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

This backport pull request has now been updated with issue and summary from the original commit.

[mlbridge]

Webrevs

• 00: Full (7e100b0b)

[tstuefe]

Since the back port was not clean, could you describe what the delta is?

[tstuefe]

Hi Frederic,

It looks okay.

If we have the time, to be sure, could you please do the following test: apply the following patch:

diff --git a/hotspot/src/share/vm/runtime/thread.cpp b/hotspot/src/share/vm/runtime/thread.cpp
index e6586c40cb..7fc4efbb0b 100644
--- a/hotspot/src/share/vm/runtime/thread.cpp
+++ b/hotspot/src/share/vm/runtime/thread.cpp
@@ -3329,6 +3329,8 @@ void Threads::threads_do(ThreadClosure* tc) {
   // If CompilerThreads ever become non-JavaThreads, add them here
 }
 
+volatile char* AAAA = NULL;
+
 jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
 
   extern void JDK_Version_init();
@@ -3503,6 +3505,11 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
     ShouldNotReachHere();
   }
 
+
+  if (UseNewCode) {
+    (*AAAA) = 'A';
+  }
+
   // Always call even when there are not JVMTI environments yet, since environments
   // may be attached late and JVMTI must track phases of VM execution
   JvmtiExport::enter_start_phase();

and run this without and without this backport.

Then test this by running java -XX:+UnlockDiagnosticVMOptions -XX:+UseNewCode -version

I expect:

• without the backport: a "silent-like" crash without a hs-err file
• with the backport: a crash with a hs-err file

Then we know for sure this works. Thank you!

[gnu-andrew]

Patch looks clean code-wise. The delta is mostly copyright headers (including os_windows.hpp already having a newer date thanks to JDK-8183925) and indentation. I do notice there is an additional change to the formatting of HOTSPOT_JNI_CREATEJAVAVM_ENTRY which makes it match the context of the patch. I don't see a problem with that.

[openjdk]

⚠️ @fthevenet 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.

[gnu-andrew]

Since the back port was not clean, could you describe what the delta is?

Yes, it is good form in these cases to describe what changes you had to make in the backport. Otherwise, the reviewers have to work this out for themselves and make assumptions as to why you made certain decisions.

[gnu-andrew]

I'd like Thomas' ack on this one as well.
/reviewers 2 reviewer

[openjdk]

@gnu-andrew
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[tstuefe]

Good. Thanks for the speedy backport work.

[tstuefe]

BTW, @gnu-andrew, should we do something about the failing security tests that make everything red? Maybe at least exclude them?

[fthevenet]

Since the back port was not clean, could you describe what the delta is?

Yes, it is good form in these cases to describe what changes you had to make in the backport. Otherwise, the reviewers have to work this out for themselves and make assumptions as to why you made certain decisions.

Conflicts arose from changes that were introduced in the target at a latter date and therefore not present in the original patch. I opted to keep things the same as they currently were and only add new lines from the patch, with three notable exceptions:

1. When the patch explicitly removed code that was present in the target
2. I picked the most recent copyright date (for some files that means the one from the target and others the one from the patch) though I'm not sure this is correct or if it matters much.
3. I used the target's indentation for HOTSPOT_JNI_CREATEJAVAVM_ENTRY rather that the source's because it felt less jarring.

[fthevenet]

/approval request The motivation for this backport is to help reduce the number of occurrences where a VM might terminate immediately without writing an error log Windows, making root cause analysis very difficult in such cases.
Backport is not clean because of conflicting changes introduced subsequently to the original patch.
Risk is low.

[fthevenet]

NB: I am planning on conducting the tests suggested by @tstuefe before I integrate this.

[openjdk]

@fthevenet
8074860: The approval request has been created successfully.

[gnu-andrew]

/approve yes

[openjdk]

@gnu-andrew
8074860: The approval request has been approved.

[openjdk]

@fthevenet This change now passes all automated pre-integration checks.

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

8074860: Structured Exception Catcher missing around CreateJavaVM on Windows

Add __try/__except around JNI_CreateJavaVM

Reviewed-by: andrew, stuefe

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 (@gnu-andrew, @tstuefe) 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).

[gnu-andrew]

BTW, @gnu-andrew, should we do something about the failing security tests that make everything red? Maybe at least exclude them?

I was hoping there was a simple solution to fixing them, but I can't see anything obvious. Frederic, did you get anywhere? I can propose a temporary exclusion so it's not failing every PR.

[fthevenet]

To follow-up: I completed the test proposed by @tstuefe and I can confirm that:

• Provoking a crash with this backport results in an hs_err being written.
• Provoking a crash without this backport results in nothing but "Segmentation fault" being written to std_err and nothing more (e.g. no hs_err report).

[fthevenet]

/integrate

[openjdk]

@fthevenet
Your change (at version 7e100b0) is now ready to be sponsored by a Committer.

[tstuefe]

/sponsor

[openjdk]

Going to push as commit ae516a3.

[openjdk]

@tstuefe @fthevenet Pushed as commit ae516a3.

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