Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278753: Runtime crashes with access violation during JNI_CreateJavaVM call #7206

Closed
wants to merge 1 commit into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Jan 25, 2022

Please review,
When jlink with --compress=2, zip is used to compress the files while doing copy. The user case failed to load zip.dll, since zip.dll is not set in PATH. This failure is after we get NULL from GetModuleHandle("zip.dll"), then do LoadLibrary("zip.dll") will have same result.
The fix is calling load_zip_library of ClassLoader first --- if zip library already loaded just return the cached handle for following usage, if not, load zip library and cached the handle.

Tests: tier1,4,7 in test
Manually tested user case, and checked output of jimage list for jlinked files using --compress=2.

Thanks
Yumin


Progress

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

Issue

  • JDK-8278753: Runtime crashes with access violation during JNI_CreateJavaVM call

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7206

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 25, 2022

👋 Welcome back minqi! 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
Copy link

openjdk bot commented Jan 25, 2022

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

  • build
  • hotspot

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.

@openjdk openjdk bot added build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jan 25, 2022
@yminqi yminqi marked this pull request as ready for review January 25, 2022 00:24
@openjdk openjdk bot added the rfr Pull request is ready for review label Jan 25, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 25, 2022

Webrevs

@dholmes-ora
Copy link
Member

This needs reviewing by the jimage folk too.

/label add core-libs

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 25, 2022
@openjdk
Copy link

openjdk bot commented Jan 25, 2022

@dholmes-ora
The core-libs label was successfully added.

@dholmes-ora
Copy link
Member

Hi Yumin,

So let me see if I have this clear.

  • The jimage code was using the OS code (dlopen/loadlibrary etc) to try and load the zip library when needed.
  • The VM, which is always loaded first, always used to load the zip library unconditionally, hence the OS would simply return the JVM's zip handle to the jimage code.
  • When we changed the VM to only load the zip library if needed (not realizing jimage may also need it) then the jimage code would now only succeed if the zip library was in the appropriate lookup paths for the OS.
  • The fix is to change the jimage code so that it asks the JVM for the zip library, as the JVM is always setup correctly to find it.

Does that sum it up?
Thanks.

@yminqi
Copy link
Contributor Author

yminqi commented Jan 25, 2022

  • The jimage code was using the OS code (dlopen/loadlibrary etc) to try and load the zip library when needed.
    Yes. The zip library has to be in PATH.
  • The VM, which is always loaded first, always used to load the zip library unconditionally, hence the OS would simply return the JVM's zip handle to the jimage code.
    Yes. After the fix, jimage will use the version that JVM is using.
  • When we changed the VM to only load the zip library if needed (not realizing jimage may also need it) then the jimage code would now only succeed if the zip library was in the appropriate lookup paths for the OS.
    Yes. When in JVM, zip library was always loaded (before https://bugs.openjdk.java.net/browse/JDK-8237750) so jimage in fact get the loaded zip handle from JVM. Unless user set PATH(other than jdk(jre)\bin) to contain the "zip.dll | libzip.so | libzip.dylib" then jimage will load and use that version. After this fix, jimage will use the same version as jvm.
  • The fix is to change the jimage code so that it asks the JVM for the zip library, as the JVM is always setup correctly to find it.
    Yes.

Thanks for taking a detail look.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay as long as there are no concerns from jimage folk about usage scenarios we may not be aware of.

Thanks,
David

@openjdk
Copy link

openjdk bot commented Jan 25, 2022

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

8278753: Runtime crashes with access violation during JNI_CreateJavaVM call

Reviewed-by: dholmes, 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 92 new commits pushed to the master branch:

  • ed826f2: 8280397: Factor out task queue statistics printing
  • 8a3cca0: 8280784: VM_Cleanup unnecessarily processes all thread oops
  • 973dda5: 8280804: Parallel: Remove unused variables in PSPromotionManager::drain_stacks_depth
  • 55f180f: 8280004: DCmdArgument::parse_value() should handle NULL input
  • 178ac74: 8251466: test/java/io/File/GetXSpace.java fails on Windows with mapped network drives.
  • a1d1e47: 8280823: Remove NULL check in DumpTimeClassInfo::is_excluded
  • 094db1a: 8277948: AArch64: Print the correct native stack if -XX:+PreserveFramePointer when crash
  • 7857405: 8280744: Allow SuppressWarnings to be used in all declaration contexts
  • 40a2ce2: 8270476: Make floating-point test infrastructure more lambda and method reference friendly
  • 6d242e4: 8280835: jdk/javadoc/tool/CheckManPageOptions.java depends on source hierarchy
  • ... and 82 more: https://git.openjdk.java.net/jdk/compare/293fb46f7cd28f2a08055e3eb8ec9459d64e9688...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jan 25, 2022
Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, under what circumstances would, before https://bugs.openjdk.java.net/browse/JDK-8237750, we ever hit the LoadLibrary in imageDecompressor.cpp? Did this ever work? Was there ever a scenario where the JVM was not involved and hence the zip.dll was not loaded already?

For me, the code looks good unless I miss a scenario where we don't have the JVM loaded already at this point.

@yminqi
Copy link
Contributor Author

yminqi commented Jan 25, 2022

I'm curious, under what circumstances would, before https://bugs.openjdk.java.net/browse/JDK-8237750, we ever hit the LoadLibrary in imageDecompressor.cpp? Did this ever work? Was there ever a scenario where the JVM was not involved and hence the zip.dll was not loaded already?

For me, the code looks good unless I miss a scenario where we don't have the JVM loaded already at this point.

Thanks for review. Before 8237750, the zip library is always loaded so jimage just get the handle of the loaded zip by calling . After that, zip is loaded at need, so if jvm does not use zip, it will not be loaded. Unfortunately, it does not realize that jimage is using this zip, so it failed to get the handle. But there exists a case, if the zip is in PATH, the following fix 8244495 used LoadLibrary("zip.dll") for a rescue. If zip.dll is not in PATH, the call still failed to load zip. This is the current issue.

So far, if user loaded zip from native code before jimage code is executed ( I could not image a scenario how it can happen), the GetModuleHandle("zip.dll") may return the handle to it, but it does not surely it is for the "zip.dll" --- if multiple instances exist, the returned handle is not guaranteed the one you want.

With this change, if user loads zip from native code (with different version), JVM does not sense that, it will still load zip from $JDK or $JRE, and jimage still uses handle returned from JVM. The only case is JVM failed to load zip library:

if (_zip_handle == NULL) {
vm_exit_during_initialization("Unable to load zip library", path);
}

You cannot make any progress on the failure.

@tstuefe
Copy link
Member

tstuefe commented Jan 26, 2022

Thanks for the explanation, @yminqi. Change looks good.

@AlanBateman
Copy link
Contributor

I think this looks okay but I think @JimLaskey and/or @sundararajana should look at this because it creates a dependency on a JVM_* function. I'm trying to think if there are any interop issues when using jrtfs. Jim/Sundar can correct me but I think we are okay there because a tool on say JDK 8 (or 11 or 17) that accesses a JDK 19 run-time image will use the BasicImageReader and won't use libjimage in the target VM.

@yminqi
Copy link
Contributor Author

yminqi commented Jan 26, 2022

Update: tier1,tier4 passed tier7 failed on: test/hotspot/jtreg/serviceability/sa/ClhsdbThreadContext.java That is not related to the change since it is not using zip.

@dcubed-ojdk
Copy link
Member

dcubed-ojdk commented Jan 26, 2022

Your Tier7 failure is likely this known bug:

JDK-8280601 ClhsdbThreadContext.java test is triggering codecache related assert in PointerFinder.find()
https://bugs.openjdk.java.net/browse/JDK-8280601

@magicus
Copy link
Member

magicus commented Jan 28, 2022

/label remove build

@openjdk openjdk bot removed the build build-dev@openjdk.org label Jan 28, 2022
@openjdk
Copy link

openjdk bot commented Jan 28, 2022

@magicus
The build label was successfully removed.

@yminqi
Copy link
Contributor Author

yminqi commented Jan 28, 2022

Thanks to @AlanBateman, @JimLaskey or @sundararajana Could you have a look and comment?

Thanks.

@yminqi
Copy link
Contributor Author

yminqi commented Feb 3, 2022

Since no further update, I will integrate tomorrow.

@yminqi
Copy link
Contributor Author

yminqi commented Feb 3, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Feb 3, 2022

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

  • 86c24b3: 8240908: RetransformClass does not know about MethodParameters attribute
  • 1f92660: 8281057: Fix doc references to overriding in JLS
  • 010965c: 8281023: NMT integration into pp debug command does not work
  • 63a00a0: 8272777: Clean up remaining AccessController warnings in test library
  • 5ab22e8: 8276990: Memory leak in invoker.c fillInvokeRequest() during JDI operations
  • fe547ea: 8280956: Re-examine copyright headers on files in src/java.desktop/macosx/native/libawt_lwawt/awt/a11y
  • a95ee5a: 8065422: Trailing dot in hostname causes TLS handshake to fail with SNI disabled
  • a46307a: Merge
  • 2531c33: 8278871: [JVMCI] assert((uint)reason < 2* _trap_hist_limit) failed: oob
  • fe0118f: 8279662: serviceability/sa/ClhsdbScanOops.java can fail do to unexpected GC
  • ... and 150 more: https://git.openjdk.java.net/jdk/compare/293fb46f7cd28f2a08055e3eb8ec9459d64e9688...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 3, 2022
@openjdk openjdk bot closed this Feb 3, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 3, 2022
@openjdk
Copy link

openjdk bot commented Feb 3, 2022

@yminqi Pushed as commit cda9c30.

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

@yminqi yminqi deleted the jdk-8278753 branch February 3, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
6 participants