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

[William Kemper]

On Tue, 8 Jul 2025 14:09:25 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

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

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp line 274:

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

s/mids/midst

src/hotspot/share/gc/shenandoah/shenandoahAsserts.cpp line 275:

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

s/still still/still

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/26187#discussion_r2193102294
PR Review Comment: https://git.openjdk.org/jdk/pull/26187#discussion_r2193102793