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

@@ -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<uintptr_t>(block);
+  uintptr_t index_loc = block_addr + offset_of(Block, _active_index);
+  static_assert(sizeof(size_t) == sizeof(intptr_t), "assumption");
+  return static_cast<size_t>(SafeFetchN(reinterpret_cast<intptr_t*>(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<oop*>(ptr), block_alignment);
+  uintptr_t section_start = align_down(reinterpret_cast<uintptr_t>(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<intptr_t>(owner);
-  for (unsigned i = 0; i < section_count; ++i, section += section_size) {
-    Block* candidate = reinterpret_cast<Block*>(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<intptr_t*>(owner_loc), 0) == owner_addr) {
+      return reinterpret_cast<Block*>(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;

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();

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();

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,9 +530,7 @@ TEST_VM_F(OopStorageTest, bulk_allocation) {
   }
 }
 
-#ifndef DISABLE_GARBAGE_ALLOCATION_STATUS_TESTS
-TEST_VM_F(OopStorageTest, invalid_pointer) {
-  {
+TEST_VM_F(OopStorageTest, invalid_malloc_pointer) {
   char* mem = NEW_C_HEAP_ARRAY(char, 1000, mtInternal);
   oop* ptr = reinterpret_cast<oop*>(align_down(mem + 250, sizeof(oop)));
   // Predicate returns false for some malloc'ed block.
@@ -528,14 +538,27 @@ TEST_VM_F(OopStorageTest, invalid_pointer) {
   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: