RFR: 8340945: Ubsan: oopStorage.cpp:374:8: runtime error: applying non-zero offset 18446744073709551168 to null pointer

[Matthias Baesken]

On Sat, 28 Sep 2024 05:20:23 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

> Please review this change to the OopStorage handling of storage block lookup,
> now being more careful about pointer arithmetic to avoid UB.
> 
> As an initial cleanup, renamed OopStorage::find_block_or_null to
> block_for_ptr, for consistency with the Block function that implements it.
> Also moved the precondition assert that the argument is non-null into the
> Block function, where the requirement is located.
> 
> Changed OopStorage::Block::block_for_ptr to avoid pointer arithmetic that
> might invoke UB, instead converting the pointer argument to uintptr_t and
> performing arithmetic on it. Also fixed its description in the header file.
> 
> Similarly changed OopStorage::Block::active_index_safe to avoid pointer
> arithmetic, instead converting to uintptr_t for arithmetic.  This avoids
> potential problems when the Block argument is a "false positive" from
> block_for_ptr. 
> 
> Changed OopStorage::allocation_status to check up front for a null argument,
> immediately returning INVALID_ENTRY in that case. This avoids voilating
> block_for_ptr's precondition that the argument is non-null. Added a gtest for
> this.  Also added a gtest for the potential false-positive case.
> 
> While updating gtests, removed #ifndef DISABLE_GARBAGE_ALLOCATION_STATUS_TESTS.
> That macro was included when these tests were first added, because some tests
> needed to be disabled on Windows, due to SafeFetchN in gtest context not working
> on that platform. That was later fixed by JDK-8185734. The conditional #define
> of that macro in test_oopStorage.cpp was removed, but the no longer needed
> #ifndef was inadvertently not removed.
> 
> Testing: mach5 tier1-5
> Locally (linux-x64) reproduced the reported ubsan failure, and verified it no
> longer reproduces with these changes.
> 
> While working on this change I noticed a related issue.  The recently added
> OopStorage::print_containing doesn't verify the block is not a false positive
> before using it as a block.  I'll file a JBS issue for this.

Marked as reviewed by mbaesken (Reviewer).

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

PR Review: https://git.openjdk.org/jdk/pull/21240#pullrequestreview-2347863489