8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297 #26187

tstuefe · 2025-07-08T14:09:25Z

Hi Shenandoah devs, may I please have thoughts and reviews for this fix.

This is a fix for a problem that was uncovered during my ongoing work on Metaspace use-after-free recognition (JDK-8340297). I would like to get this fix into the codebase before eventually pushing JDK-8340297, since I want the fixes to be separate from the patch that introduces stricter testing.

For details about this issue, please see the Jira description and comment. The short version is:

class gets redefined, and therefore its Klass is discarded and its mirror oop left to be eventually collected. Mirror oop's Klass field is nulled out before.
but we may be in the middle of an evacuation. So the mirror oop may be forwarded. In that case, class redefinition (correctly) nulls out the forwardee's Klass reference.
But that leaves the old forwarded mirror oop untouched, and that still carries the now invalid Klass pointer in its Klass field. That we now notice with JDK-8340297. So, ShenandoaAsserts::assert_correct complains about an invalid Klass reference when processing the old mirror oop.

I thought hard about this, but I believe this is benign, since no code should be accessing the old mirror oop's Klass field, it should always resolve the forwardee first. Class redefinition itself gets the oop address to null out from an OopHandle in Klass, which should contain the forwardee's address if the mirror was forwarded, which should ensure that only the forwardee is nulled out, never the forwarded.

Therefore I just changed the assertions.

But I would like to hear other people's opinion on this.

Note why we never saw this before: Before JDK-8340297 (as in, now), the only check ShenandoaAssert makes for Klass validity is Metaspace::contains, which is weak. A reclaimed Klass pointer will still point to Metaspace, so ShenandoaAssert does not complain. Metadata::is_valid() is similarly weak now, since it basically just checks a byte in the object for not null; pretty much any non-null garbage will satisfy this condition.

Progress

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

Issue

JDK-8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297 (Bug - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)

Contributors

Aleksey Shipilev <shade@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26187/head:pull/26187
$ git checkout pull/26187

Update a local copy of the PR:
$ git checkout pull/26187
$ git pull https://git.openjdk.org/jdk.git pull/26187/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26187

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26187.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-07-08T14:09:52Z

👋 Welcome back stuefe! 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 · 2025-07-08T14:09:55Z

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

8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297

Co-authored-by: Aleksey Shipilev <shade@openjdk.org>
Reviewed-by: shade

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

bf3cfbe: 8351487: [ubsan] jvmti.h runtime error: load of value which is not a valid value
2a53f5a: 8361754: New test runtime/jni/checked/TestCharArrayReleasing.java can cause disk full errors
f67e435: 8361447: [REDO] Checked version of JNI ReleaseArrayElements needs to filter out known wrapped arrays
... and 21 more: https://git.openjdk.org/jdk/compare/fe264676337cdef0d7477b0b57ff9d2fe8f9fc0f...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 · 2025-07-08T14:10:43Z

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

hotspot-gc
shenandoah

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.

tstuefe · 2025-07-08T14:55:22Z

Ping @shipilev or @rkennke?

mlbridge · 2025-07-08T14:59:49Z

Webrevs

01: Full - Incremental (c33cf248)
00: Full (3d07a9d9)

shipilev · 2025-07-08T15:21:34Z

Only looked at this briefly, but that sounds like what I did here: #21064 -- but forgot to complete it?

shipilev · 2025-07-08T16:30:18Z

Only looked at this briefly, but that sounds like what I did here: #21064 -- but forgot to complete it?

Yes, it looks like it is! The sequence of events you describe seems to also match JDK-8340364, which I think we can close as duplicate, as I have not been able to convince even myself is a real bug, not just overzealous verification. So, what do you want to do? I can finish up #21064 and test it with your Metaspace assertion patches, if you want.

earthling-amzn · 2025-07-08T17:43:06Z

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp

-    Metadata* klass = obj->metadata_field(java_lang_Class::klass_offset());
+    // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference
+    // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been
+    // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference


s/mids/midst

earthling-amzn · 2025-07-08T17:43:24Z

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp

+    // During class redefinition the old Klass gets reclaimed and the old mirror oop's Klass reference
+    // nulled out (hence the "klass != nullptr" condition below). However, the mirror oop may have been
+    // forwarded if we are in the mids of an evacuation. In that case, the forwardee's Klass reference
+    // is nulled out. The old, forwarded, still still carries the old invalid Klass pointer. It will be


s/still still/still

earthling-amzn · 2025-07-08T17:50:00Z

The explanation and the changes make sense to me. Thank you!

…n-invalid-mirror-object-after-JDK-8340297

ExE-Boss · 2025-07-09T10:26:05Z

There’s a missing “n” in the PR and JBS issue titles:

-JDK-8361342: Sheandoah evacuation may assert on invalid mirror object after JDK-8340297
+JDK-8361342: Shenandoah evacuation may assert on invalid mirror object after JDK-8340297

tstuefe · 2025-07-09T11:41:18Z

Only looked at this briefly, but that sounds like what I did here: #21064 -- but forgot to complete it?

Yes, it looks like it is! The sequence of events you describe seems to also match JDK-8340364, which I think we can close as duplicate, as I have not been able to convince even myself is a real bug, not just overzealous verification. So, what do you want to do? I can finish up #21064 and test it with your Metaspace assertion patches, if you want.

Thanks, let me just finish this, I need the closure after burning time on this non-bug :-)

Note that I did not fix up the use of the array mirror klass since I thought it should always be stable. We never redefine array classes. But then it does not hurt either.

I am also preparing a small improvement patch for print_obj, as per our discussion in #26117 yesterday, and will put the printing of the mirror klasses there, too.

tstuefe · 2025-07-09T11:41:39Z

The explanation and the changes make sense to me. Thank you!

Thanks for the review, @earthling-amzn !

shipilev · 2025-07-09T15:16:19Z

Thanks, let me just finish this, I need the closure after burning time on this non-bug :-)

Sure, OK. Please pick up most of the hunks from #21064, including the array mirrors then? I don't see a need to write a full paragraph of text explaining why we need to check for forwarded objects. A simple sentence is enough, and the rest can go into the bug description.

tstuefe · 2025-07-10T06:28:09Z

/contributor add @shipilev

openjdk · 2025-07-10T06:28:31Z

@tstuefe
Contributor Aleksey Shipilev <shade@openjdk.org> successfully added.

tstuefe · 2025-07-10T06:28:41Z

@shipilev ok done

shipilev

Looks fine to me!

tstuefe · 2025-07-10T08:38:28Z

Thanks Aleksey! @earthling-amzn are you happy with this version, too?

start

a9c3c58

openjdk bot changed the title ~~JDK-8361342: Sheandoah evacuation may assert on invalid mirror object after JDK-8340297~~ 8361342: Sheandoah evacuation may assert on invalid mirror object after JDK-8340297 Jul 8, 2025

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Jul 8, 2025

also fix ShenandoahVerifier

3d07a9d

tstuefe marked this pull request as ready for review July 8, 2025 14:54

openjdk bot added the rfr Pull request is ready for review label Jul 8, 2025

earthling-amzn reviewed Jul 8, 2025

View reviewed changes

tstuefe added 3 commits July 9, 2025 07:16

Merge branch 'master' into JDK-8361342-Sheandoah-evacuation-asserts-o…

e8d7c5a

…n-invalid-mirror-object-after-JDK-8340297

Merge branch 'master' into JDK-8361342-Sheandoah-evacuation-asserts-o…

4d23ef7

…n-invalid-mirror-object-after-JDK-8340297

Also check array klass

fba14a0

tstuefe changed the title ~~8361342: Sheandoah evacuation may assert on invalid mirror object after JDK-8340297~~ 8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297 Jul 10, 2025

Alekseys additions

c33cf24

shipilev approved these changes Jul 10, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 10, 2025

8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297 #26187

Are you sure you want to change the base?

8361342: Shenandoah: Evacuation may assert on invalid mirror object after JDK-8340297 #26187

Conversation

tstuefe commented Jul 8, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Jul 8, 2025

Uh oh!

openjdk bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 8, 2025

Uh oh!

tstuefe commented Jul 8, 2025

Uh oh!

mlbridge bot commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev commented Jul 8, 2025

Uh oh!

shipilev commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

earthling-amzn Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

earthling-amzn Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

earthling-amzn commented Jul 8, 2025

Uh oh!

ExE-Boss commented Jul 9, 2025

Uh oh!

tstuefe commented Jul 9, 2025

Uh oh!

tstuefe commented Jul 9, 2025

Uh oh!

shipilev commented Jul 9, 2025

Uh oh!

tstuefe commented Jul 10, 2025

Uh oh!

openjdk bot commented Jul 10, 2025

Uh oh!

tstuefe commented Jul 10, 2025

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

tstuefe commented Jul 10, 2025

Uh oh!

Uh oh!

tstuefe commented Jul 8, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 8, 2025 •

edited

Loading

mlbridge bot commented Jul 8, 2025 •

edited

Loading

shipilev commented Jul 8, 2025 •

edited

Loading