RFR: 8330174: Protection zone for easier detection of accidental zero-nKlass use [v5]

[Thomas Stuefe]

On Thu, 23 Jan 2025 06:15:24 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> If we wrongly decode an nKlass of `0`, and the nKlass encoding base is not NULL (typical for most cases that run with CDS enabled), the resulting pointer points to the start of the Klass encoding range. That area is readable. If CDS is enabled, it will be at the start of the CDS metadata archive. If CDS is off, it is at the start of the class space. 
>> 
>> Now, both CDS and class space allocate a safety buffer at the start to prevent Klass structures from being located there. However, that memory is still readable, so we can read garbage data from that area. In the case of CDS, that area is just 16 bytes, after that come real data. Since Klass is large, most accesses will read beyond the 16-byte zone.
>> 
>> We should protect the first page in the narrow Klass encoding range to make analysis of errors like this easier. Especially in release builds where decode_not_null does not assert. We already use a similar technique in the heap, and most OSes protect the zero page for the same reason.
>> 
>> This patch does that. Now, decoding an `0` nKlass and then using the result `Klass` - calling virtual functions or accessing members - crashes right away.
>> 
>> Additionally, the patch provides a helpful output in the register/stack section, e.g:
>> 
>> 
>> RDI=0x0000000800000000 points into nKlass protection zone
>> 
>> 
>> 
>> Testing: 
>> - GHAs.
>> - I tested the patch manually on x64 Linux for both CDS on, CDS off and zero-based encoding, CDS off and non-zero-based encoding.
>> - I tested manually on Windows x64
>> - I also prepared an automatic gtest, but that needs some preparatory work on the gtest suite first to work (see https://bugs.openjdk.org/browse/JDK-8348029)
>> 
>> -- Update 2024-01-22 -- 
>> I added a jtreg test that is more thorough than a gtest (also scans the produced hs-err file)
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fix whitespace error

> > It might be easier if we introduce a new "core" region called "protection" that's 16MB in size, and allocate that before the rw region in the output buffer. We never map this region so it doesn't need to be stored in the archive file.
> > Let me try this out and see if it works.
> 
> Hi Thomas, please try this out:
> 
> [master...iklam:jdk:8330174-protection-zone-ioi-contributions](https://github.com/openjdk/jdk/compare/master...iklam:jdk:8330174-protection-zone-ioi-contributions)
> 
> It passes all CDS tests. You can see the gap:
> 
> ```
> $ java -Xlog:cds -XX:ArchiveRelocationMode=0 --version | egrep '(Mapped)|(_rs)'
> [0.017s][info][cds] Reserved archive_space_rs [0x0000000800000000 - 0x0000000801000000] (16777216) bytes
> [0.017s][info][cds] Reserved class_space_rs   [0x0000000801000000 - 0x0000000841000000] (1073741824) bytes
> [0.017s][info][cds] Mapped static  region #0 at base 0x0000000800001000 top 0x0000000800557000 (ReadWrite)
> [0.017s][info][cds] Mapped static  region #1 at base 0x0000000800557000 top 0x0000000800dee000 (ReadOnly)
> [0.017s][info][cds] Mapped static  region #2 at base 0x000079ff9c021000 top 0x000079ff9c056000 (Bitmap)
> ```
> 
> You'd need to add code to disable all RWX access in 0x800000000 ~ 0x800001000.

I like this, @iklam, thanks. I am on vacation currently; will try this when I'm back.

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

PR Comment: https://git.openjdk.org/jdk/pull/23190#issuecomment-2661262145