Fix scudo MTE tests.
r.android.com/2108505 was intended to fix a crash in Scudo in
the case where the stack depot, region info or ring buffer were
unreadable. However, it also ended up introducing a number of bugs into
the code. It failed to call __scudo_get_error_info if the page at the
fault address was unreadable. This can happen in legitimate crash cases
if a primary allocation was close to the boundary of a mapped region,
or if the allocation was a secondary allocation with guard pages. It
also used long as the type for tags, whereas Scudo expects it to be
char. In combination this ended up causing most of the MTE tests to
fail. Therefore, mostly revert that change.
Fix the original crash by null checking the pointers returned by
AllocAndReadFully before proceeding with the rest of the function.
Bug: 233720136
Change-Id: I04d70d2abffaa35fe315d15d9224f9b412a9825d
diff --git a/debuggerd/libdebuggerd/scudo.cpp b/debuggerd/libdebuggerd/scudo.cpp
index 9483e59..5d861f8 100644
--- a/debuggerd/libdebuggerd/scudo.cpp
+++ b/debuggerd/libdebuggerd/scudo.cpp
@@ -14,11 +14,6 @@
* limitations under the License.
*/
-#include <stdint.h>
-#include <unistd.h>
-
-#include <vector>
-
#include "libdebuggerd/scudo.h"
#include "libdebuggerd/tombstone.h"
@@ -30,92 +25,57 @@
#include "tombstone.pb.h"
-bool ScudoCrashData::SetErrorInfo(unwindstack::Memory* process_memory,
- const ProcessInfo& process_info) {
+std::unique_ptr<char[]> AllocAndReadFully(unwindstack::Memory* process_memory, uint64_t addr,
+ size_t size) {
+ auto buf = std::make_unique<char[]>(size);
+ if (!process_memory->ReadFully(addr, buf.get(), size)) {
+ return std::unique_ptr<char[]>();
+ }
+ return buf;
+}
+
+ScudoCrashData::ScudoCrashData(unwindstack::Memory* process_memory,
+ const ProcessInfo& process_info) {
if (!process_info.has_fault_address) {
- return false;
+ return;
}
- std::vector<char> stack_depot(__scudo_get_stack_depot_size());
- if (!process_memory->ReadFully(process_info.scudo_stack_depot, stack_depot.data(),
- stack_depot.size())) {
- return false;
+ auto stack_depot = AllocAndReadFully(process_memory, process_info.scudo_stack_depot,
+ __scudo_get_stack_depot_size());
+ auto region_info = AllocAndReadFully(process_memory, process_info.scudo_region_info,
+ __scudo_get_region_info_size());
+ auto ring_buffer = AllocAndReadFully(process_memory, process_info.scudo_ring_buffer,
+ __scudo_get_ring_buffer_size());
+ if (!stack_depot || !region_info || !ring_buffer) {
+ return;
}
- std::vector<char> region_info(__scudo_get_region_info_size());
- if (!process_memory->ReadFully(process_info.scudo_region_info, region_info.data(),
- region_info.size())) {
- return false;
- }
- std::vector<char> ring_buffer(__scudo_get_ring_buffer_size());
- if (!process_memory->ReadFully(process_info.scudo_ring_buffer, ring_buffer.data(),
- ring_buffer.size())) {
- return false;
- }
-
- uintptr_t page_size = getpagesize();
untagged_fault_addr_ = process_info.untagged_fault_address;
- uintptr_t fault_page = untagged_fault_addr_ & ~(page_size - 1);
+ uintptr_t fault_page = untagged_fault_addr_ & ~(PAGE_SIZE - 1);
- // Attempt to get 16 pages before the fault page and 16 pages after.
- constexpr size_t kExtraPages = 16;
- std::vector<char> memory(page_size * (kExtraPages * 2 + 1));
-
- // Read faulting page first.
- size_t memory_index = kExtraPages;
- if (!process_memory->ReadFully(fault_page, &memory[memory_index * page_size], page_size)) {
- return false;
+ uintptr_t memory_begin = fault_page - PAGE_SIZE * 16;
+ if (memory_begin > fault_page) {
+ return;
}
- // Attempt to read the pages after the fault page, stop as soon as we
- // fail to read.
- uintptr_t read_addr = fault_page;
- if (!__builtin_add_overflow(fault_page, page_size, &read_addr)) {
- memory_index++;
- for (size_t i = 0; i < kExtraPages; i++, memory_index++) {
- if (!process_memory->ReadFully(read_addr, &memory[memory_index * page_size], page_size)) {
- break;
- }
- if (__builtin_add_overflow(read_addr, page_size, &read_addr)) {
- break;
- }
- }
- }
- uintptr_t memory_end = read_addr;
-
- // Attempt to read the pages before the fault page, stop as soon as we
- // fail to read.
- memory_index = kExtraPages;
- if (fault_page > 0) {
- read_addr = fault_page - page_size;
- for (size_t i = 0; i < kExtraPages; i++, memory_index--) {
- if (!process_memory->ReadFully(read_addr, &memory[(memory_index - 1) * page_size],
- page_size)) {
- break;
- }
- if (read_addr == 0) {
- memory_index--;
- break;
- }
- read_addr -= page_size;
- }
- }
- size_t start_memory_index = memory_index;
- uintptr_t memory_begin = fault_page - (kExtraPages - memory_index) * page_size;
-
- std::vector<long> memory_tags((memory_end - memory_begin) / kTagGranuleSize);
- read_addr = memory_begin;
- for (size_t i = 0; i < memory_tags.size(); i++) {
- memory_tags[i] = process_memory->ReadTag(read_addr);
- read_addr += kTagGranuleSize;
+ uintptr_t memory_end = fault_page + PAGE_SIZE * 16;
+ if (memory_end < fault_page) {
+ return;
}
- __scudo_get_error_info(
- &error_info_, process_info.maybe_tagged_fault_address, stack_depot.data(), region_info.data(),
- ring_buffer.data(), &memory[start_memory_index * page_size],
- reinterpret_cast<const char*>(memory_tags.data()), memory_begin, memory_end - memory_begin);
+ auto memory = std::make_unique<char[]>(memory_end - memory_begin);
+ for (auto i = memory_begin; i != memory_end; i += PAGE_SIZE) {
+ process_memory->ReadFully(i, memory.get() + i - memory_begin, PAGE_SIZE);
+ }
- return true;
+ auto memory_tags = std::make_unique<char[]>((memory_end - memory_begin) / kTagGranuleSize);
+ for (auto i = memory_begin; i != memory_end; i += kTagGranuleSize) {
+ memory_tags[(i - memory_begin) / kTagGranuleSize] = process_memory->ReadTag(i);
+ }
+
+ __scudo_get_error_info(&error_info_, process_info.maybe_tagged_fault_address, stack_depot.get(),
+ region_info.get(), ring_buffer.get(), memory.get(), memory_tags.get(),
+ memory_begin, memory_end - memory_begin);
}
bool ScudoCrashData::CrashIsMine() const {