RFR: 8340297: Use-after-free recognition for metaspace and class space [v3]

[Johan Sjölen]

On Sat, 16 Aug 2025 05:06:27 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> This patch will give us use-after-free recognition for Metaspace and Class space. 
>> 
>> Currently, checks for Klass validity typically only perform a variation of `Metaspace::contains` and some other basic tests. These checks won't find cases where the Klass had been prematurely freed (e.g., after class redefinition), nor cases of unloaded classes if the underlying metaspace chunks have not been uncommitted, which is quite common. 
>> 
>> The patch also provides us with improved analysis methods in case we encounter problems. E.g., answering whether the Klass had been redefined or unloaded.
>> 
>> The implementation aims to be simple, fast, and safe against false positives. There is a small but non-null chance that we could get false negatives, but that cannot be avoided.
>> 
>> How this works:
>> 
>> - In `class Metadata`, we introduce a 32-bit token that holds the type of the object (1). It replaces the old "is_valid" field of the same size. That one was of limited use since any non-null garbage in those four bytes would be read as valid.
>> - To check a Metadata for validity, the token is checked. Checks are done with SafeFetch, so they can be done with questionable pointers (e.g. into uncommitted metaspace after class unloading)
>> - When metaspace is freed (bulk free after class unloading), the released chunks are zapped, destroying all tokens in the area.
>> - When metaspace is freed (prematurely, e.g., after class redefinition), the released blocks are zapped.
>> - The new checks replace Metadata::is_valid and supplement some other metadata checks done in GCs
>> 
>> Testing: The patch has been extensively tested manually, at Oracle, and SAP. Tests were thorough to not only catch errors in the patch, but also to see if the patch would uncover a lot of existing sleeper bugs. So far, we only found a single bug in Shenandoah.
>> 
>> Note: I did not yet hook up the new test to c1/c2 compiled code (there are already unimplemented functions for that). That is possible, but left for a later RFE.
>
> Thomas Stuefe has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
> 
>  - Merge branch 'openjdk:master' into JDK-8340297-Metaspace-API-for-checking-if-address-is-in-use
>  - merge master
>  - copyrights
>  - fix big-endian problem on AIX
>  - Update klass.cpp
>  - Update metaspace.hpp
>  - Update metaspace.hpp
>  - Update metaspace.hpp
>  - fix rebase error
>  - fix mac build
>  - ... and 1 more: https://git.openjdk.org/jdk/compare/57210af9...d9696c76

src/hotspot/share/memory/metaspace.cpp line 1051:

> 1049: }
> 1050: 
> 1051: bool Metaspace::metadata_is_live(const Metadata* md, FailureHint* hint) {

Why can't `FailureHint` have an arm called 'no_failure' and just skip it as  an out-parameter, having the failure be the return value?

src/hotspot/share/memory/metaspace.hpp line 162:

> 160:     unknown = 0,
> 161:     // outside class space/metaspace
> 162:     outside,                          // 1

I don't see why we need to write the value of each enum arm, can't our IDEs figure that out for us if necessary :-)?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25891#discussion_r2281422124
PR Review Comment: https://git.openjdk.org/jdk/pull/25891#discussion_r2281427915