RFR: JDK-8331953> ubsan: metaspaceShared.cpp:1305:57: runtime error: applying non-zero offset 12849152 to null pointer

[Thomas Stuefe]

On Thu, 9 May 2024 06:28:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> Reported by @MBaesken at SAP. ubsan complains about this line:
> 
> 
> const size_t ccs_begin_offset = align_up(base_address + archive_space_size,
>                                            class_space_alignment) - base_address;
> 
> 
> `base_address` here is the wish address, with NULL being an explicitly allowed value that indicates "no preference". The line calculates the offset of the class space within the future combined CDS+class-space mapping. Ubsan complains about `base_address` being possibly NULL.
> 
> Ubsan is missing the point somewhat. The addition is not a problem, on our platforms at least.
> 
> However, it highlights a slight incorrectness (which it did not notice): `base_address` is the wish for the future base of the Klass range. That wish is not guaranteed to be fulfilled; the eventual start of the Klass range could be somewhere else. Therefore, calculating the class space offset with an alignment based on that wish address is wrong. It always worked in practice since `base_address` was always aligned to class_space_alignment (16MB).
> 
> Hence, the fix is simple: We just make the alignment requirement for the base address explicit. When running with class space, we now assert that `base_address` is aligned to  class space alignment (as well as CDS core region alignment, but that is much smaller). Since `base_address` is calculated either from a hard-wired default or from the `SharedBaseAddress` user input, and both are ensured to be properly aligned, that assert should never fire.
> 
> Then, the offending calculation can be simplified by removing the base address from it.

@MBaesken , and possibly @iklam or @calvinccheung ?

src/hotspot/share/cds/metaspaceShared.cpp line 1309:

> 1307: 
> 1308:   const size_t total_range_size =
> 1309:       archive_space_size + gap_size + class_space_size;

Removed the align_up, since it is unnecessary since both (archive_space_size + gap_size) and (class space size) are already aligned to the much stronger class space alignment. It also makes no sense to align the total Klass range size at all, to anything (if anything, it should just be page aligned).

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

PR Comment: https://git.openjdk.org/jdk/pull/19152#issuecomment-2102330986
PR Review Comment: https://git.openjdk.org/jdk/pull/19152#discussion_r1595229354