Skip to content
This repository has been archived by the owner. It is now read-only.

8258236: Segfault in ClassListParser::resolve_indy dumping static AppCDS archive #30

Closed
wants to merge 1 commit into from
Closed

8258236: Segfault in ClassListParser::resolve_indy dumping static AppCDS archive #30

wants to merge 1 commit into from

Conversation

calvinccheung
Copy link
Member

@calvinccheung calvinccheung commented Dec 15, 2020

Please review this change for JDK 16.

In ClassListParser::resolve_indy, if a class has previously failed verification, don't proceed with resolve indy for that class to avoid dereferencing a null cpcache pointer.

Passed tiers 1,2,3,4 tests.


Progress

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

Issue

  • JDK-8258236: Segfault in ClassListParser::resolve_indy dumping static AppCDS archive

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk16 pull/30/head:pull/30
$ git checkout pull/30

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 15, 2020

👋 Welcome back ccheung! 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 Dec 15, 2020

@calvinccheung The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Dec 15, 2020
@calvinccheung
Copy link
Member Author

calvinccheung commented Dec 15, 2020

/label remove hotspot
/label add hotspot-runtime

@openjdk openjdk bot removed the hotspot label Dec 15, 2020
@openjdk
Copy link

openjdk bot commented Dec 15, 2020

@calvinccheung
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.java.net label Dec 15, 2020
@openjdk
Copy link

openjdk bot commented Dec 15, 2020

@calvinccheung
The hotspot-runtime label was successfully added.

@calvinccheung calvinccheung marked this pull request as ready for review Dec 15, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 15, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 15, 2020

Webrevs

cl4es
cl4es approved these changes Dec 15, 2020
Copy link
Member

@cl4es cl4es left a comment

Fix and tests looks good!

@openjdk
Copy link

openjdk bot commented Dec 15, 2020

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

8258236: Segfault in ClassListParser::resolve_indy dumping static AppCDS archive

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

  • b97fe6c: 8258419: RSA cipher buffer cleanup
  • 1f556d2: 8258380: [JVMCI] don't clear InstalledCode reference when unloading JVMCI nmethods
  • e7aa5fe: 8258427: Problem List some tests related to FileDialog for MacOS
  • e911351: 8258140: Update @jls tags in java.base for renamed/renumbered sections
  • ce36aea: 8257822: C2 crashes with SIGFPE due to a division that floats above its zero check
  • fa1cbb4: 8258404: Restore stacktrace reuse after 8258094
  • 7ff9c85: 8258242: Type profile pollution occurs when memory segments of different kinds are used
  • 09e8675: 8255381: com/sun/jdi/EATests.java should not suspend graal threads

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.

➡️ 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 Dec 15, 2020
if (SystemDictionaryShared::has_class_failed_verification(ik)) {
// don't attempt to resolve indy on classes that has previously failed verification
return;
}
MetaspaceShared::try_link_class(ik, THREAD);
Copy link
Contributor

@coleenp coleenp Dec 15, 2020

Choose a reason for hiding this comment

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

Doesn't the check for failing verification belong after try_link_class(), which runs the verifier?

Copy link
Member Author

@calvinccheung calvinccheung Dec 16, 2020

Choose a reason for hiding this comment

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

Linking attempt was done for the class and was marked as failed verification prior to getting into ClassListParser::resolve_indy.

Consider the following class list (from the test case):

BadInvokeDynamic
@lambda-proxy BadInvokeDynamic run ()Ljava/lang/Runnable; ()V REF_invokeStatic BadInvokeDynamic lambda$doTest$0 ()V ()V

The BadInvokeDynamic was linked via the following code path:

	MetaspaceShared::try_link_class() at metaspaceShared.cpp:1,161 0x7ffff63b1c10	
	MetaspaceShared::preload_classes() at metaspaceShared.cpp:1,125 0x7ffff63b1a42	
	MetaspaceShared::preload_and_dump() at metaspaceShared.cpp:1,047 0x7ffff63b1557	
	Threads::create_vm() at thread.cpp:3,809 0x7ffff666b3f9	
	JNI_CreateJavaVM_inner() at jni.cpp:3,769 0x7ffff604b1be	
	JNI_CreateJavaVM() at jni.cpp:3,852 0x7ffff604b4c3	
	InitializeJVM() at java.c:1,536 0x7ffff7fcd664	
	JavaMain() at java.c:416 0x7ffff7fca252	
	ThreadJavaMain() at java_md.c:651 0x7ffff7fd0f77	
	start_thread() at 0x7ffff79b0ea5

It was marked as failed verification in MetaspaceShared::try_link_class in the following:

    ik->link_class(THREAD);
    if (HAS_PENDING_EXCEPTION) {
      ResourceMark rm(THREAD);
      log_warning(cds)("Preload Warning: Verification failed for %s",
                    ik->external_name());
      CLEAR_PENDING_EXCEPTION;
      SystemDictionaryShared::set_class_has_failed_verification(ik);
      _has_error_classes = true;
    }

During the processing of the @lambda-proxy in the class list, the ClassListParser::resolve_indy would be called and the check is for checking if the caller class has failed verification before.

Copy link
Contributor

@coleenp coleenp Dec 16, 2020

Choose a reason for hiding this comment

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

Okay so you're trying to catch a previous link time failure. Why do you expect the next try_link_class call to never fail?

@@ -467,6 +467,10 @@ void ClassListParser::resolve_indy(Symbol* class_name_symbol, TRAPS) {
Klass* klass = SystemDictionary::resolve_or_fail(class_name_symbol, class_loader, protection_domain, true, THREAD); // FIXME should really be just a lookup
Copy link
Contributor

@coleenp coleenp Dec 15, 2020

Choose a reason for hiding this comment

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

If an exception is unexpected, this should be CHECK not THREAD.

Copy link
Member Author

@calvinccheung calvinccheung Dec 16, 2020

Choose a reason for hiding this comment

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

I'll try CHECK and do more testing before posting an updated webrev.

Thanks for your review.

Copy link
Contributor

@coleenp coleenp Dec 16, 2020

Choose a reason for hiding this comment

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

For this change, don't change it to CHECK in case something new will fail, and any new exceptions will caught by checking k != NULL. It just looks strange but can be cleaned up later.

@calvinccheung
Copy link
Member Author

calvinccheung commented Dec 16, 2020

Fix and tests looks good!

@cl4es Thanks for your review.

Copy link
Contributor

@coleenp coleenp left a comment

I just have questions. The change as-is is good.

@@ -467,6 +467,10 @@ void ClassListParser::resolve_indy(Symbol* class_name_symbol, TRAPS) {
Klass* klass = SystemDictionary::resolve_or_fail(class_name_symbol, class_loader, protection_domain, true, THREAD); // FIXME should really be just a lookup
Copy link
Contributor

@coleenp coleenp Dec 16, 2020

Choose a reason for hiding this comment

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

For this change, don't change it to CHECK in case something new will fail, and any new exceptions will caught by checking k != NULL. It just looks strange but can be cleaned up later.

if (SystemDictionaryShared::has_class_failed_verification(ik)) {
// don't attempt to resolve indy on classes that has previously failed verification
return;
}
MetaspaceShared::try_link_class(ik, THREAD);
Copy link
Contributor

@coleenp coleenp Dec 16, 2020

Choose a reason for hiding this comment

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

Okay so you're trying to catch a previous link time failure. Why do you expect the next try_link_class call to never fail?

@calvinccheung
Copy link
Member Author

calvinccheung commented Dec 16, 2020

For this change, don't change it to CHECK in case something new will fail, and any new exceptions will caught by checking k != NULL. It just looks strange but can be cleaned up later.

I was thinking the same - do the clean up later. Initial local testing looks good - no new cds/appcds tests failure.

Okay so you're trying to catch a previous link time failure. Why do you expect the next try_link_class call to never fail?

Previous link time failure was recorded via SystemDictionaryShared::set_class_has_failed_verification(ik).
try_link_class will not result in an exception because there's a if (HAS_PENDING_EXCEPTION) block which calls CLEAR_PENDING_EXCEPTION.

@calvinccheung
Copy link
Member Author

calvinccheung commented Dec 16, 2020

/integrate

@openjdk openjdk bot closed this Dec 16, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 16, 2020
@openjdk
Copy link

openjdk bot commented Dec 16, 2020

@calvinccheung Since your change was applied there have been 8 commits pushed to the master branch:

  • b97fe6c: 8258419: RSA cipher buffer cleanup
  • 1f556d2: 8258380: [JVMCI] don't clear InstalledCode reference when unloading JVMCI nmethods
  • e7aa5fe: 8258427: Problem List some tests related to FileDialog for MacOS
  • e911351: 8258140: Update @jls tags in java.base for renamed/renumbered sections
  • ce36aea: 8257822: C2 crashes with SIGFPE due to a division that floats above its zero check
  • fa1cbb4: 8258404: Restore stacktrace reuse after 8258094
  • 7ff9c85: 8258242: Type profile pollution occurs when memory segments of different kinds are used
  • 09e8675: 8255381: com/sun/jdi/EATests.java should not suspend graal threads

Your commit was automatically rebased without conflicts.

Pushed as commit b5a3a5b.

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

@calvinccheung calvinccheung deleted the 8258236-resolve_indy branch Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.java.net integrated Pull request has been integrated
3 participants