8286277: CDS VerifyError when calling clone() on object array #8737

iklam · 2022-05-16T22:49:24Z

Problem:

When verifying an invokevirtual bytecode that calls a protected method in a superclass of the current class, the verifier requires that the object being invoked must be a subtype of the current class. This prevents invocation of protected methods in an unrelated class. For example, the following is disallowed -- the type InvokeCloneInvalid cannot invoke clone() on a String:

super public class InvokeCloneInvalid
    version 52:0
{
  public static Method test:"(Ljava/lang/String;)V"
    stack 1 locals 1
  {
    aload_0;
    invokevirtual Method "java/lang/Object".clone:"()Ljava/lang/Object;";
    return;
  }
}

However, there's a special case where invoking Object.clone() is allowed on all object arrays. Here's an example:

super public class InvokeCloneValid
    version 52:0
{
  public static Method test:"([Ljava/lang/Object;)V"
    stack 1 locals 1
  {
    aload_0;
    invokevirtual Method "java/lang/Object".clone:"()Ljava/lang/Object;";
    return;
  }

Before this PR. the Verifier would first do the assignability check. When a failure is encountered, the Verifier then checks for the special case and ignores the failure accordingly.

In the above case, the assignability check will fail because the object being invoked (an Object array) is not assignable to the current class (InvokeCloneValid)

The problem is that CDS remembers all the assignability checks and tries to replay them at runtime. Therefore, it will re-check the assignability of Object Array to InvokeCloneValid, and throw a VerifyError as a result.

Proposed Fix

In ClassVerifier::verify_invoke_instructions(), check the special case first, before doing the assignability check. This way, such invalid-but-ignored assignability checks are no longer performed and thus won't be remembered by CDS.

Testing

Tiers 1 - 4, plus 3 new test cases.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

JDK-8286277: CDS VerifyError when calling clone() on object array

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to a292dc39
Calvin Cheung (@calvinccheung - Reviewer) ⚠️ Review applies to a292dc39

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8737

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

Using diff file

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

bridgekeeper · 2022-05-16T22:51:04Z

👋 Welcome back iklam! 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-05-16T22:53:07Z

@iklam 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-05-16T23:12:07Z

Webrevs

dholmes-ora · 2022-05-17T06:36:40Z

test/hotspot/jtreg/runtime/verifier/InvokeCloneValid.jasm

+    invokevirtual Method "java/lang/Object".clone:"()Ljava/lang/Object;";
+    return;
+  }
+}


Missing newline at end of line.

dholmes-ora

Is the component type of the array already loaded and verified at this point? My only concern would be that the is_assignable check might load and verify the class as a side-effect, and we have now skipped that. ??

hseigel · 2022-05-17T13:44:46Z

The is_assignable_check() may or may not load the array class depending on what it needs to check assignability. But neither the verifier nor the JVM has any dependency on the array class being loaded. Note that if this were not a protected access or if name_in_supers() returned null then the is_assignable_check() would not get called. Also note that the verifier tries to avoid loading classes.

Also, the assignability check may cause the array class to get loaded but would not cause it to get linked or verified.

dholmes-ora · 2022-05-18T06:57:17Z

Thanks Harold! @hseigel

openjdk · 2022-05-18T06:59:04Z

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

8286277: CDS VerifyError when calling clone() on object array

Reviewed-by: dholmes, ccheung

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

ef7a9f8: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate
ac274c4: 8286956: Loom: Define test groups for development/porting use
5d8d6da: 8286972: Support the new loop induction variable related PopulateIndex IR node on x86
8122466: 8287113: JFR: Periodic task thread uses period for method sampling events
940e94f: 8285739: disable EscapeBarrier deopt for virtual threads
110d906: 8287103: java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java fails with Xcomp
8040aa0: 8286908: ECDSA signature should not return parameters
689f80c: 8287153: Whitespace typos in HeapRegion class
c906591: 8286391: Address possibly lossy conversions in jdk.compiler
88018c4: 8287150: Remove HeapRegion::block_start_const declaration without definition
... and 95 more: https://git.openjdk.java.net/jdk/compare/a31130fd4056907edcb420761722c629a33273eb...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.

calvinccheung · 2022-05-19T19:13:47Z

test/hotspot/jtreg/runtime/cds/appcds/dynamicArchive/VerifyObjArrayCloneTest.java

+ * @build sun.hotspot.WhiteBox
+  * @build InvokeCloneValid InvokeCloneInvalid VerifyObjArrayCloneTestApp


Please align line 32 with line 31.

calvinccheung

Looks good. Just one nit.

iklam · 2022-05-23T16:21:39Z

Thanks @calvinccheung and @dholmes-ora for the review, and @hseigel for confirming the fix.
/integrate

openjdk · 2022-05-23T16:23:23Z

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

ef7a9f8: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate
ac274c4: 8286956: Loom: Define test groups for development/porting use
5d8d6da: 8286972: Support the new loop induction variable related PopulateIndex IR node on x86
8122466: 8287113: JFR: Periodic task thread uses period for method sampling events
940e94f: 8285739: disable EscapeBarrier deopt for virtual threads
110d906: 8287103: java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java fails with Xcomp
8040aa0: 8286908: ECDSA signature should not return parameters
689f80c: 8287153: Whitespace typos in HeapRegion class
c906591: 8286391: Address possibly lossy conversions in jdk.compiler
88018c4: 8287150: Remove HeapRegion::block_start_const declaration without definition
... and 95 more: https://git.openjdk.java.net/jdk/compare/a31130fd4056907edcb420761722c629a33273eb...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-23T16:23:43Z

@iklam Pushed as commit 646c8aa.

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

8286277: CDS VerifyError when calling clone() on object array

d450849

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 16, 2022

fixed trailing spaces

7121754

iklam marked this pull request as ready for review May 16, 2022 23:07

openjdk bot added the rfr Pull request is ready for review label May 16, 2022

dholmes-ora reviewed May 17, 2022

View reviewed changes

Adding newline at end of file

a292dc3

dholmes-ora approved these changes May 18, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 18, 2022

calvinccheung reviewed May 19, 2022

View reviewed changes

calvinccheung approved these changes May 19, 2022

View reviewed changes

@calvinccheung review -- fixed alignment

7f2e232

openjdk bot added the integrated Pull request has been integrated label May 23, 2022

openjdk bot closed this May 23, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8286277: CDS VerifyError when calling clone() on object array #8737

8286277: CDS VerifyError when calling clone() on object array #8737

iklam commented May 16, 2022 •

edited by openjdk bot

bridgekeeper bot commented May 16, 2022

openjdk bot commented May 16, 2022

mlbridge bot commented May 16, 2022 •

edited

dholmes-ora May 17, 2022

iklam May 17, 2022

dholmes-ora left a comment

hseigel commented May 17, 2022

dholmes-ora commented May 18, 2022

openjdk bot commented May 18, 2022 •

edited

calvinccheung May 19, 2022

calvinccheung left a comment

iklam commented May 23, 2022

openjdk bot commented May 23, 2022

openjdk bot commented May 23, 2022

		* @build sun.hotspot.WhiteBox
		* @build InvokeCloneValid InvokeCloneInvalid VerifyObjArrayCloneTestApp

8286277: CDS VerifyError when calling clone() on object array #8737

8286277: CDS VerifyError when calling clone() on object array #8737

Conversation

iklam commented May 16, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 16, 2022

openjdk bot commented May 16, 2022

mlbridge bot commented May 16, 2022 • edited

Webrevs

dholmes-ora May 17, 2022

Choose a reason for hiding this comment

iklam May 17, 2022

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

hseigel commented May 17, 2022

dholmes-ora commented May 18, 2022

openjdk bot commented May 18, 2022 • edited

calvinccheung May 19, 2022

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

iklam commented May 23, 2022

openjdk bot commented May 23, 2022

openjdk bot commented May 23, 2022

iklam commented May 16, 2022 •

edited by openjdk bot

mlbridge bot commented May 16, 2022 •

edited

openjdk bot commented May 18, 2022 •

edited