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

8261395: C1 crash "cannot make java calls from the native compiler" #3913

Closed
wants to merge 9 commits into from

Conversation

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented May 7, 2021

If a nest host and a nest member are associated with different protection domains it can lead to execution of Java code (to validate the "new" protection domain) during a nestmate access check, if nest membership verification has not yet been performed. This will cause assertion or guarantee failures if executed by a JIT compiler thread during access checks.

After much discussion and trying different solutions it was decided that the existing logic for nest membership validation unnecessarily tries to resolve constant-pool entries, when it suffices that the symbolic entry in the constant-pool has the same name as the class being checked. Given this check occurs after we have verified the nest host and the purported member are loaded by the same classloader and in the same runtime package, there can only be one class with the name of the member, and that is the member class. Hence resolution of the constant-pool entry serves no purpose but introduces the complexity of dealing with exceptions and avoiding Java code execution in compiler threads.

@iklam contributed to an earlier version of the fix, and devised the initial testcase approach.
@coleenp also contributed to an earlier version of the fix.

Thanks to both Coleen and Ioi for their insights, discussions and contributions.

Testing:

  • the new test
  • tiers 1-3

Thanks,
David


Progress

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

Issue

  • JDK-8261395: C1 crash "cannot make java calls from the native compiler"

Reviewers

Contributors

  • Ioi Lam <iklam@openjdk.org>
  • Coleen Phillimore <coleenp@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3913

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

Using diff file

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

dholmes-ora added 6 commits May 6, 2021
…ss_at but can use resolved_klass_at. Now exceptions are truly impossible.
This avoids any possibility of class loading or executing any Java
code and removes all possibility of exceptions in this part of the
nestmate verification process.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 7, 2021

👋 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.

@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented May 7, 2021

/label remove hotspot
/label add hotspot-runtime
/contributor add @iklam
/contributor add @coleenp

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@dholmes-ora The hotspot label was not set.

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@dholmes-ora
The hotspot-runtime label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@dholmes-ora
Contributor Ioi Lam <iklam@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@dholmes-ora
Contributor Coleen Phillimore <coleenp@openjdk.org> successfully added.

@dholmes-ora dholmes-ora marked this pull request as ready for review May 7, 2021
@openjdk openjdk bot added the rfr label May 7, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 7, 2021

Webrevs

iklam
iklam approved these changes May 7, 2021
Copy link
Member

@iklam iklam left a comment

LGTM. Just a suggestion for an assert.

src/hotspot/share/oops/instanceKlass.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

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

8261395: C1 crash "cannot make java calls from the native compiler"

Co-authored-by: Ioi Lam <iklam@openjdk.org>
Co-authored-by: Coleen Phillimore <coleenp@openjdk.org>
Reviewed-by: iklam, hseigel, coleenp

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

  • 616244f: 8266937: Remove Compile::reshape_address
  • 974b9f7: 8266773: Release VM is broken with GCC 9 after 8214237
  • f6c5a6b: 8266784: java/text/Collator/RuleBasedCollatorTest.java fails with jtreg 6
  • 1356116: 8266456: Replace direct TKit.run() calls with jdk.jpackage.test.Annotations.Test annotation
  • dfe8833: 8266783: java/lang/reflect/Proxy/DefaultMethods.java fails with jtreg 6
  • 995e956: 8266225: jarsigner is using incorrect security property to show weakness of certs
  • 0a12605: 8265448: (zipfs): Reduce read contention in ZipFileSystem
  • acf02ed: 8208237: Re-examine defmeth tests and update as needed
  • ac0287f: 8266770: Clean pending exception before running dynamic CDS dump
  • 7a0a57c: 8266820: micro java/nio/SelectorWakeup.java has wrong copyright header
  • ... and 30 more: https://git.openjdk.java.net/jdk/compare/3af4efdfcfbbb52d38415374083c66c9e7b22604...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 label May 7, 2021
hseigel
hseigel approved these changes May 7, 2021
Copy link
Member

@hseigel hseigel left a comment

LGTM. Thanks for fixing this.
Harold

Copy link

@coleenp coleenp left a comment

Looks good but I think you should remove the conversion of Thread to JavaThread because it's also going to conflict with your later change.

src/hotspot/share/oops/instanceKlass.cpp Outdated Show resolved Hide resolved
super(parent);
}

static byte[] getBytesForClass(String name) throws IOException {
Copy link

@coleenp coleenp May 7, 2021

There's a ClassUnloadCommon library call for this.

Copy link
Member Author

@dholmes-ora dholmes-ora May 7, 2021

Thanks. I copied this from another test. If there's an existing utility I will switch to it.

@coleenp
Copy link

@coleenp coleenp commented May 7, 2021

/contributor remove @coleenp
I don't really get credit for this newer version, which is even better than my previous suggestions.

@openjdk
Copy link

@openjdk openjdk bot commented May 7, 2021

@coleenp Only the author (@dholmes-ora) is allowed to issue the contributor command.

dholmes-ora added 3 commits May 9, 2021
- need to add java.security.manager=allow just to be safe
- simplify logic to read classfile from disk (ref ClassUnloadCommon.getClassData())
@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented May 9, 2021

Thanks for the reviews Coleen, Ioi and Harold.

Minor updates in place based on Coleen's feedback and suggestions. Please re-review.

@coleenp we wouldn't have arrived at this simplified name-check-only version without all of the discussions and investigations of how to fix the more complex version. So credit remains where credit is due. :)

Thanks,
David

Copy link

@coleenp coleenp left a comment

Looks good to me!

@@ -93,7 +76,8 @@
// Check for target class
if (name.startsWith(TARGET)) {
try {
byte[] buff = getBytesForClass(name);
String clzFile = name.replaceAll("\\.", "/") + ".class";
byte[] buff = getResourceAsStream(clzFile).readAllBytes();
Copy link

@coleenp coleenp May 11, 2021

There's a ClassUnloadCommon.getBytesForClass(name) that you could have called that does essentially what these two lines do. Maybe it's different enough to have this, so it's up to you to change it.

iklam
iklam approved these changes May 11, 2021
@dholmes-ora
Copy link
Member Author

@dholmes-ora dholmes-ora commented May 12, 2021

/integrate

@openjdk openjdk bot closed this May 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 12, 2021

@dholmes-ora Since your change was applied there have been 46 commits pushed to the master branch:

  • 3c47cab: 8261034: improve jcmd GC.class_histogram to support parallel
  • ed32e02: 8241187: ToolBox::grep should allow for negative filtering
  • cc03734: 8266925: Add a test to verify that hidden class's members are not statically invocable
  • 271a0c7: 8047218: [TEST_BUG] java/awt/FullScreen/AltTabCrashTest/AltTabCrashTest.java fails with exception
  • 1a0ff28: 8255035: Update BCEL to Version 6.5.0
  • 57c6ba6: 8266822: Rename MetaspaceShared::is_old_class to be more explicit about what "old" means
  • 616244f: 8266937: Remove Compile::reshape_address
  • 974b9f7: 8266773: Release VM is broken with GCC 9 after 8214237
  • f6c5a6b: 8266784: java/text/Collator/RuleBasedCollatorTest.java fails with jtreg 6
  • 1356116: 8266456: Replace direct TKit.run() calls with jdk.jpackage.test.Annotations.Test annotation
  • ... and 36 more: https://git.openjdk.java.net/jdk/compare/3af4efdfcfbbb52d38415374083c66c9e7b22604...master

Your commit was automatically rebased without conflicts.

Pushed as commit e828a93.

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

@dholmes-ora dholmes-ora deleted the 8261395 branch May 12, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 8/05/2021 12:08 am, Harold Seigel wrote:

On Fri, 7 May 2021 05:26:41 GMT, David Holmes <dholmes at openjdk.org> wrote:

If a nest host and a nest member are associated with different protection domains it can lead to execution of Java code (to validate the "new" protection domain) during a nestmate access check, if nest membership verification has not yet been performed. This will cause assertion or guarantee failures if executed by a JIT compiler thread during access checks.

After much discussion and trying different solutions it was decided that the existing logic for nest membership validation unnecessarily tries to resolve constant-pool entries, when it suffices that the symbolic entry in the constant-pool has the same name as the class being checked. Given this check occurs after we have verified the nest host and the purported member are loaded by the same classloader and in the same runtime package, there can only be one class with the name of the member, and that is the member class. Hence resolution of the constant-pool entry serves no purpose but introduces the complexity of dealing with exceptions and avoiding Java code execution in compiler threads.

@iklam contributed to an earlier version of the fix, and devised the initial testcase approach.
@coleenp also contributed to an earlier version of the fix.

Thanks to both Coleen and Ioi for their insights, discussions and contributions.

Testing:
- the new test
- tiers 1-3

Thanks,
David

LGTM. Thanks for fixing this.

Thanks for the review Harold!

David

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

Hi Coleen,

On 12/05/2021 9:34 am, Coleen Phillimore wrote:

On Sun, 9 May 2021 22:10:24 GMT, David Holmes <dholmes at openjdk.org> wrote:

David Holmes has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:

- Update test per Coleen's comments:
- need to add java.security.manager=allow just to be safe
- simplify logic to read classfile from disk (ref ClassUnloadCommon.getClassData())
- Merge branch 'master' into 8261395-nestmember
- Per Coleen: Reverse arguments order for has_nest_member so we can revert the introduction of "current".
- Fix typo
- Completely simpified the has_nest_member check to only use names.
This avoids any possibility of class loading or executing any Java
code and removes all possibility of exceptions in this part of the
nestmate verification process.
- Add test
- Tweak logging message
- 8261395: C1 crash "cannot make java calls from the native compiler"
- First cleanup: when we have a resolved klass we don't need to use klass_at but can use resolved_klass_at. Now exceptions are truly impossible.

Looks good to me!

test/hotspot/jtreg/runtime/Nestmates/protectionDomain/TestDifferentProtectionDomains.java line 80:

78: try {
79: String clzFile = name.replaceAll("\\.", "/") + ".class";
80: byte[] buff = getResourceAsStream(clzFile).readAllBytes();

There's a ClassUnloadCommon.getBytesForClass(name) that you could have called that does essentially what these two lines do. Maybe it's different enough to have this, so it's up to you to change it.

It would be "wrong" to actually call that as it calls
getResourceAsStream on a different ClassLoader. So I used the same logic
but applied to the custom classloader instance.

Thanks for the review.

David
-----

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants