RFR: 8331920: ubsan: g1CardSetContainers.inline.hpp:266:5: runtime error: index 2 out of bounds for type 'G1CardSetHowl::ContainerPtr [2]' reported

[Axel Boldt-Christmas]

On Mon, 20 May 2024 08:17:41 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi all,
> 
> Please review this change to improve access to G1CardSetContainer VLA elements. Instead of  straightforward indexing, we obscure access through a base pointer and offsets to reduce the possibility of UB. 
> 
> Testing: Tier 1-3
>               - Local testing on Mac with `--enable-ubsan`.

The only scary change to me was changing the object layout of `G1CardSetHowl`. But I can find nothing wrong with it, but I also do not understand why it originally was `[2]` instead of `[1]`.

src/hotspot/share/gc/g1/g1CardSetContainers.hpp line 243:

> 241:   // VLA implementation.
> 242:   ContainerPtr _buckets[1];
> 243:   // Do not add class member variables beyond this point.

Was there any dependency to this being `[2]` at some point? I tried to find if any usage of G1CardSetHowl depends on it but could not.

src/hotspot/share/gc/g1/g1CardSetContainers.inline.hpp line 267:

> 265: inline G1CardSetHowl::ContainerPtr* G1CardSetHowl::container_addr(EntryCountType index) {
> 266:   assert(index < _num_entries, "precondition");
> 267:   return const_cast<ContainerPtr*>(buckets() + index);

While the implementation is very short here, one could chose to implement one in terms of the other.
Suggestion:

  return const_cast<ContainerPtr*>(const_cast<const G1CardSetHowl*>(this)->container_addr(index));

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

Marked as reviewed by aboldtch (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19300#pullrequestreview-2067694758
PR Review Comment: https://git.openjdk.org/jdk/pull/19300#discussion_r1607742200
PR Review Comment: https://git.openjdk.org/jdk/pull/19300#discussion_r1607763364