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

[Kim Barrett]

On Mon, 30 Sep 2024 08:29:17 GMT, Thomas Schatzl <tschatzl 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 tschatzl (Reviewer).

Thanks for reviews @tschatzl and @MBaesken .

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

PR Comment: https://git.openjdk.org/jdk/pull/21240#issuecomment-2394012889