From feb6a830e291ff71e2803e37be6c35c237f7c1cf Mon Sep 17 00:00:00 2001 From: Kim Barrett Date: Fri, 4 Oct 2024 15:58:22 +0000 Subject: [PATCH] 8340945: Ubsan: oopStorage.cpp:374:8: runtime error: applying non-zero offset 18446744073709551168 to null pointer Reviewed-by: tschatzl, mbaesken --- src/hotspot/share/gc/shared/oopStorage.cpp | 35 +++++++----- src/hotspot/share/gc/shared/oopStorage.hpp | 2 +- .../share/gc/shared/oopStorage.inline.hpp | 5 +- .../gtest/gc/shared/test_oopStorage.cpp | 57 +++++++++++++------ 4 files changed, 66 insertions(+), 33 deletions(-) diff --git a/src/hotspot/share/gc/shared/oopStorage.cpp b/src/hotspot/share/gc/shared/oopStorage.cpp index 568888ac7d9..2373d6b1d93 100644 --- a/src/hotspot/share/gc/shared/oopStorage.cpp +++ b/src/hotspot/share/gc/shared/oopStorage.cpp @@ -300,7 +300,12 @@ void OopStorage::Block::set_active_index(size_t index) { size_t OopStorage::Block::active_index_safe(const Block* block) { STATIC_ASSERT(sizeof(intptr_t) == sizeof(block->_active_index)); - return SafeFetchN((intptr_t*)&block->_active_index, 0); + // Be careful, because block could be a false positive from block_for_ptr. + assert(block != nullptr, "precondition"); + uintptr_t block_addr = reinterpret_cast(block); + uintptr_t index_loc = block_addr + offset_of(Block, _active_index); + static_assert(sizeof(size_t) == sizeof(intptr_t), "assumption"); + return static_cast(SafeFetchN(reinterpret_cast(index_loc), 0)); } unsigned OopStorage::Block::get_index(const oop* ptr) const { @@ -366,21 +371,23 @@ void OopStorage::Block::delete_block(const Block& block) { OopStorage::Block* OopStorage::Block::block_for_ptr(const OopStorage* owner, const oop* ptr) { STATIC_ASSERT(_data_pos == 0); - // Const-ness of ptr is not related to const-ness of containing block. + assert(ptr != nullptr, "precondition"); // Blocks are allocated section-aligned, so get the containing section. - oop* section_start = align_down(const_cast(ptr), block_alignment); + uintptr_t section_start = align_down(reinterpret_cast(ptr), block_alignment); // Start with a guess that the containing section is the last section, // so the block starts section_count-1 sections earlier. - oop* section = section_start - (section_size * (section_count - 1)); + size_t section_size_in_bytes = sizeof(oop) * section_size; + uintptr_t section = section_start - (section_size_in_bytes * (section_count - 1)); // Walk up through the potential block start positions, looking for // the owner in the expected location. If we're below the actual block // start position, the value at the owner position will be some oop // (possibly null), which can never match the owner. intptr_t owner_addr = reinterpret_cast(owner); - for (unsigned i = 0; i < section_count; ++i, section += section_size) { - Block* candidate = reinterpret_cast(section); - if (SafeFetchN(&candidate->_owner_address, 0) == owner_addr) { - return candidate; + for (unsigned i = 0; i < section_count; ++i, section += section_size_in_bytes) { + uintptr_t owner_loc = section + offset_of(Block, _owner_address); + static_assert(sizeof(OopStorage*) == sizeof(intptr_t), "assumption"); + if (SafeFetchN(reinterpret_cast(owner_loc), 0) == owner_addr) { + return reinterpret_cast(section); } } return nullptr; @@ -643,8 +650,7 @@ public: } }; -OopStorage::Block* OopStorage::find_block_or_null(const oop* ptr) const { - assert(ptr != nullptr, "precondition"); +OopStorage::Block* OopStorage::block_for_ptr(const oop* ptr) const { return Block::block_for_ptr(this, ptr); } @@ -771,7 +777,7 @@ static inline void check_release_entry(const oop* entry) { void OopStorage::release(const oop* ptr) { check_release_entry(ptr); - Block* block = find_block_or_null(ptr); + Block* block = block_for_ptr(ptr); assert(block != nullptr, "%s: invalid release " PTR_FORMAT, name(), p2i(ptr)); log_trace(oopstorage, ref)("%s: releasing " PTR_FORMAT, name(), p2i(ptr)); block->release_entries(block->bitmask_for_entry(ptr), this); @@ -782,7 +788,7 @@ void OopStorage::release(const oop* const* ptrs, size_t size) { size_t i = 0; while (i < size) { check_release_entry(ptrs[i]); - Block* block = find_block_or_null(ptrs[i]); + Block* block = block_for_ptr(ptrs[i]); assert(block != nullptr, "%s: invalid release " PTR_FORMAT, name(), p2i(ptrs[i])); size_t count = 0; uintx releasing = 0; @@ -989,7 +995,8 @@ bool OopStorage::delete_empty_blocks() { } OopStorage::EntryStatus OopStorage::allocation_status(const oop* ptr) const { - const Block* block = find_block_or_null(ptr); + if (ptr == nullptr) return INVALID_ENTRY; + const Block* block = block_for_ptr(ptr); if (block != nullptr) { // Prevent block deletion and _active_array modification. MutexLocker ml(_allocation_mutex, Mutex::_no_safepoint_check_flag); @@ -1137,7 +1144,7 @@ const char* OopStorage::name() const { return _name; } bool OopStorage::print_containing(const oop* addr, outputStream* st) { if (addr != nullptr) { - Block* block = find_block_or_null(addr); + Block* block = block_for_ptr(addr); if (block != nullptr && block->print_containing(addr, st)) { st->print(" in oop storage \"%s\"", name()); return true; diff --git a/src/hotspot/share/gc/shared/oopStorage.hpp b/src/hotspot/share/gc/shared/oopStorage.hpp index 96cc5a23d6a..34c980a0586 100644 --- a/src/hotspot/share/gc/shared/oopStorage.hpp +++ b/src/hotspot/share/gc/shared/oopStorage.hpp @@ -288,7 +288,7 @@ private: Block* block_for_allocation(); void log_block_transition(Block* block, const char* new_state) const; - Block* find_block_or_null(const oop* ptr) const; + Block* block_for_ptr(const oop* ptr) const; void delete_empty_block(const Block& block); bool reduce_deferred_updates(); void record_needs_cleanup(); diff --git a/src/hotspot/share/gc/shared/oopStorage.inline.hpp b/src/hotspot/share/gc/shared/oopStorage.inline.hpp index 545da0be0a7..da0926a20b6 100644 --- a/src/hotspot/share/gc/shared/oopStorage.inline.hpp +++ b/src/hotspot/share/gc/shared/oopStorage.inline.hpp @@ -184,7 +184,10 @@ public: void set_active_index(size_t index); static size_t active_index_safe(const Block* block); // Returns 0 if access fails. - // Returns null if ptr is not in a block or not allocated in that block. + // Return block of owner containing ptr, if ptr is a valid entry of owner. + // If ptr is not a valid entry of owner then returns either null or a "false + // positive" pointer; see allocation_status. + // precondition: ptr != nullptr static Block* block_for_ptr(const OopStorage* owner, const oop* ptr); oop* allocate(); diff --git a/test/hotspot/gtest/gc/shared/test_oopStorage.cpp b/test/hotspot/gtest/gc/shared/test_oopStorage.cpp index a4ac2fee66b..d55d6dbfc23 100644 --- a/test/hotspot/gtest/gc/shared/test_oopStorage.cpp +++ b/test/hotspot/gtest/gc/shared/test_oopStorage.cpp @@ -93,6 +93,18 @@ public: static void block_array_set_block_count(ActiveArray* blocks, size_t count) { blocks->_block_count = count; } + + static const oop* get_block_pointer(const Block& block, unsigned index) { + return block.get_pointer(index); + } + + static Block* new_block(const OopStorage& owner) { + return Block::new_block(&owner); + } + + static void delete_block(const Block& block) { + Block::delete_block(block); + } }; typedef OopStorage::TestAccess TestAccess; @@ -518,24 +530,35 @@ TEST_VM_F(OopStorageTest, bulk_allocation) { } } -#ifndef DISABLE_GARBAGE_ALLOCATION_STATUS_TESTS -TEST_VM_F(OopStorageTest, invalid_pointer) { - { - char* mem = NEW_C_HEAP_ARRAY(char, 1000, mtInternal); - oop* ptr = reinterpret_cast(align_down(mem + 250, sizeof(oop))); - // Predicate returns false for some malloc'ed block. - EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(ptr)); - FREE_C_HEAP_ARRAY(char, mem); - } - - { - oop obj; - oop* ptr = &obj; - // Predicate returns false for some "random" location. - EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(ptr)); - } +TEST_VM_F(OopStorageTest, invalid_malloc_pointer) { + char* mem = NEW_C_HEAP_ARRAY(char, 1000, mtInternal); + oop* ptr = reinterpret_cast(align_down(mem + 250, sizeof(oop))); + // Predicate returns false for some malloc'ed block. + EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(ptr)); + FREE_C_HEAP_ARRAY(char, mem); +} + +TEST_VM_F(OopStorageTest, invalid_random_pointer) { + oop obj; + oop* ptr = &obj; + // Predicate returns false for some "random" location. + EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(ptr)); +} + +TEST_VM_F(OopStorageTest, invalid_block_pointer) { + // Allocate a block for storage, but don't insert it into the storage. This + // also tests the false positive case of block_for_ptr where we have a + // reference to storage at just the "right" place. + const OopBlock* block = TestAccess::new_block(storage()); + ASSERT_NE(block, NULL_BLOCK); + const oop* ptr = TestAccess::get_block_pointer(*block, 0); + EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(ptr)); + TestAccess::delete_block(*block); +} + +TEST_VM_F(OopStorageTest, invalid_null_pointer) { + EXPECT_EQ(OopStorage::INVALID_ENTRY, storage().allocation_status(nullptr)); } -#endif // DISABLE_GARBAGE_ALLOCATION_STATUS_TESTS class OopStorageTest::CountingIterateClosure { public: