Free classes after objects for memory tool
Since the memory tool space calls Object::SizeOf, we need to free
classes after instances to prevent reading stale data.
Bug: 131542326
Test: art/test/testrunner/run_build_test_target.py -j50 art-gtest-heap-poisoning
Change-Id: I4b0583b2456e8db23ae3cd19ef6495bff955e7dc
diff --git a/runtime/gc/accounting/space_bitmap.cc b/runtime/gc/accounting/space_bitmap.cc
index dc223db..4029057 100644
--- a/runtime/gc/accounting/space_bitmap.cc
+++ b/runtime/gc/accounting/space_bitmap.cc
@@ -169,23 +169,28 @@
return;
}
- // TODO: rewrite the callbacks to accept a std::vector<mirror::Object*> rather than a mirror::Object**?
- constexpr size_t buffer_size = sizeof(intptr_t) * kBitsPerIntPtrT;
-#ifdef __LP64__
- // Heap-allocate for smaller stack frame.
- std::unique_ptr<mirror::Object*[]> pointer_buf_ptr(new mirror::Object*[buffer_size]);
- mirror::Object** pointer_buf = pointer_buf_ptr.get();
-#else
- // Stack-allocate buffer as it's small enough.
- mirror::Object* pointer_buf[buffer_size];
-#endif
- mirror::Object** pb = &pointer_buf[0];
-
- size_t start = OffsetToIndex(sweep_begin - live_bitmap.heap_begin_);
- size_t end = OffsetToIndex(sweep_end - live_bitmap.heap_begin_ - 1);
- CHECK_LT(end, live_bitmap.Size() / sizeof(intptr_t));
+ size_t buffer_size = sizeof(intptr_t) * kBitsPerIntPtrT;
Atomic<uintptr_t>* live = live_bitmap.bitmap_begin_;
Atomic<uintptr_t>* mark = mark_bitmap.bitmap_begin_;
+ const size_t start = OffsetToIndex(sweep_begin - live_bitmap.heap_begin_);
+ const size_t end = OffsetToIndex(sweep_end - live_bitmap.heap_begin_ - 1);
+ CHECK_LT(end, live_bitmap.Size() / sizeof(intptr_t));
+
+ if (Runtime::Current()->IsRunningOnMemoryTool()) {
+ // For memory tool, make the buffer large enough to hold all allocations. This is done since
+ // we get the size of objects (and hence read the class) inside of the freeing logic. This can
+ // cause crashes for unloaded classes since the class may get zeroed out before it is read.
+ // See b/131542326
+ for (size_t i = start; i <= end; i++) {
+ uintptr_t garbage =
+ live[i].load(std::memory_order_relaxed) & ~mark[i].load(std::memory_order_relaxed);
+ buffer_size += POPCOUNT(garbage);
+ }
+ }
+ std::vector<mirror::Object*> pointer_buf(buffer_size);
+ mirror::Object** cur_pointer = &pointer_buf[0];
+ mirror::Object** pointer_end = cur_pointer + (buffer_size - kBitsPerIntPtrT);
+
for (size_t i = start; i <= end; i++) {
uintptr_t garbage =
live[i].load(std::memory_order_relaxed) & ~mark[i].load(std::memory_order_relaxed);
@@ -194,18 +199,18 @@
do {
const size_t shift = CTZ(garbage);
garbage ^= (static_cast<uintptr_t>(1)) << shift;
- *pb++ = reinterpret_cast<mirror::Object*>(ptr_base + shift * kAlignment);
+ *cur_pointer++ = reinterpret_cast<mirror::Object*>(ptr_base + shift * kAlignment);
} while (garbage != 0);
// Make sure that there are always enough slots available for an
// entire word of one bits.
- if (pb >= &pointer_buf[buffer_size - kBitsPerIntPtrT]) {
- (*callback)(pb - &pointer_buf[0], &pointer_buf[0], arg);
- pb = &pointer_buf[0];
+ if (cur_pointer >= pointer_end) {
+ (*callback)(cur_pointer - &pointer_buf[0], &pointer_buf[0], arg);
+ cur_pointer = &pointer_buf[0];
}
}
}
- if (pb > &pointer_buf[0]) {
- (*callback)(pb - &pointer_buf[0], &pointer_buf[0], arg);
+ if (cur_pointer > &pointer_buf[0]) {
+ (*callback)(cur_pointer - &pointer_buf[0], &pointer_buf[0], arg);
}
}
diff --git a/runtime/gc/space/memory_tool_malloc_space-inl.h b/runtime/gc/space/memory_tool_malloc_space-inl.h
index f1c1cb8..ba08889 100644
--- a/runtime/gc/space/memory_tool_malloc_space-inl.h
+++ b/runtime/gc/space/memory_tool_malloc_space-inl.h
@@ -251,6 +251,12 @@
kUseObjSizeForUsable>::FreeList(
Thread* self, size_t num_ptrs, mirror::Object** ptrs) {
size_t freed = 0;
+ // Sort the pointers to free non class objects first. See b/131542326 for why this is necessary to
+ // avoid crashes.
+ std::sort(ptrs, ptrs + num_ptrs, [](mirror::Object* a, mirror::Object* b)
+ REQUIRES_SHARED(Locks::mutator_lock_) {
+ return a->IsClass() < b->IsClass();
+ });
for (size_t i = 0; i < num_ptrs; i++) {
freed += Free(self, ptrs[i]);
ptrs[i] = nullptr;