From f3d542fe9fe4b82affada29928921314375d42f8 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Thu, 5 Mar 2020 16:46:15 -0800 Subject: [PATCH] Create a debugger_process_info data structure with the process info pointers. Similar to r.android.com/1247247 I'll be adding more of them for MTE. Also, change the protocol between the crasher and crash_dump to make it easier to add new fields and change the referenced data structures without needing to worry about versioning. The version number for static executables is now always 1 (where the protocol will never change), while the version number for dynamic executables is always 4 (where the protocol can change, because the linker and crash_dump are version locked). Bug: 135772972 Change-Id: Ib4696d0544d7c87cb429aaaa15f18c3640059e16 --- debuggerd/crash_dump.cpp | 30 ++++++----- debuggerd/crasher/crasher.cpp | 6 ++- debuggerd/handler/debuggerd_handler.cpp | 67 ++++++++++++------------- debuggerd/include/debuggerd/handler.h | 13 +++-- debuggerd/protocol.h | 12 ++--- 5 files changed, 63 insertions(+), 65 deletions(-) diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 6a381453e..087ebaea8 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -264,15 +264,13 @@ static void ReadCrashInfo(unique_fd& fd, siginfo_t* siginfo, ssize_t expected_size = 0; switch (crash_info->header.version) { case 1: - expected_size = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataV1); - break; - case 2: - expected_size = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataV2); + case 3: + expected_size = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataStatic); break; - case 3: - expected_size = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataV3); + case 4: + expected_size = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataDynamic); break; default: @@ -280,25 +278,25 @@ static void ReadCrashInfo(unique_fd& fd, siginfo_t* siginfo, break; }; - if (rc != expected_size) { + if (rc < expected_size) { LOG(FATAL) << "read " << rc << " bytes when reading target crash information, expected " << expected_size; } } switch (crash_info->header.version) { - case 3: - process_info->gwp_asan_state = crash_info->data.v3.gwp_asan_state; - process_info->gwp_asan_metadata = crash_info->data.v3.gwp_asan_metadata; - FALLTHROUGH_INTENDED; - case 2: - process_info->fdsan_table_address = crash_info->data.v2.fdsan_table_address; + case 4: + process_info->fdsan_table_address = crash_info->data.d.fdsan_table_address; + process_info->gwp_asan_state = crash_info->data.d.gwp_asan_state; + process_info->gwp_asan_metadata = crash_info->data.d.gwp_asan_metadata; FALLTHROUGH_INTENDED; case 1: - process_info->abort_msg_address = crash_info->data.v1.abort_msg_address; - *siginfo = crash_info->data.v1.siginfo; + case 2: + case 3: + process_info->abort_msg_address = crash_info->data.s.abort_msg_address; + *siginfo = crash_info->data.s.siginfo; regs->reset(unwindstack::Regs::CreateFromUcontext(unwindstack::Regs::CurrentArch(), - &crash_info->data.v1.ucontext)); + &crash_info->data.s.ucontext)); break; default: diff --git a/debuggerd/crasher/crasher.cpp b/debuggerd/crasher/crasher.cpp index 304166472..a2b13a36c 100644 --- a/debuggerd/crasher/crasher.cpp +++ b/debuggerd/crasher/crasher.cpp @@ -349,7 +349,7 @@ noinline int do_action(const char* arg) { int main(int argc, char** argv) { #if defined(STATIC_CRASHER) debuggerd_callbacks_t callbacks = { - .get_abort_message = []() { + .get_process_info = []() { static struct { size_t size; char msg[32]; @@ -357,7 +357,9 @@ int main(int argc, char** argv) { msg.size = strlen("dummy abort message"); memcpy(msg.msg, "dummy abort message", strlen("dummy abort message")); - return reinterpret_cast(&msg); + return debugger_process_info{ + .abort_msg = reinterpret_cast(&msg), + }; }, .post_dump = nullptr }; diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index 8b4b6304f..84d2bd09a 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -297,10 +297,7 @@ struct debugger_thread_info { pid_t pseudothread_tid; siginfo_t* siginfo; void* ucontext; - uintptr_t abort_msg; - uintptr_t fdsan_table; - uintptr_t gwp_asan_state; - uintptr_t gwp_asan_metadata; + debugger_process_info process_info; }; // Logging and contacting debuggerd requires free file descriptors, which we might not have. @@ -344,25 +341,36 @@ static int debuggerd_dispatch_pseudothread(void* arg) { fatal_errno("failed to create pipe"); } - // ucontext_t is absurdly large on AArch64, so piece it together manually with writev. - uint32_t version = 3; - constexpr size_t expected = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataV3); + uint32_t version; + ssize_t expected; + // ucontext_t is absurdly large on AArch64, so piece it together manually with writev. + struct iovec iovs[4] = { + {.iov_base = &version, .iov_len = sizeof(version)}, + {.iov_base = thread_info->siginfo, .iov_len = sizeof(siginfo_t)}, + {.iov_base = thread_info->ucontext, .iov_len = sizeof(ucontext_t)}, + }; + + if (thread_info->process_info.fdsan_table) { + // Dynamic executables always use version 4. There is no need to increment the version number if + // the format changes, because the sender (linker) and receiver (crash_dump) are version locked. + version = 4; + expected = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataDynamic); + + iovs[3] = {.iov_base = &thread_info->process_info, + .iov_len = sizeof(thread_info->process_info)}; + } else { + // Static executables always use version 1. + version = 1; + expected = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataStatic); + + iovs[3] = {.iov_base = &thread_info->process_info.abort_msg, .iov_len = sizeof(uintptr_t)}; + } errno = 0; if (fcntl(output_write.get(), F_SETPIPE_SZ, expected) < static_cast(expected)) { fatal_errno("failed to set pipe buffer size"); } - struct iovec iovs[] = { - {.iov_base = &version, .iov_len = sizeof(version)}, - {.iov_base = thread_info->siginfo, .iov_len = sizeof(siginfo_t)}, - {.iov_base = thread_info->ucontext, .iov_len = sizeof(ucontext_t)}, - {.iov_base = &thread_info->abort_msg, .iov_len = sizeof(uintptr_t)}, - {.iov_base = &thread_info->fdsan_table, .iov_len = sizeof(uintptr_t)}, - {.iov_base = &thread_info->gwp_asan_state, .iov_len = sizeof(uintptr_t)}, - {.iov_base = &thread_info->gwp_asan_metadata, .iov_len = sizeof(uintptr_t)}, - }; - ssize_t rc = TEMP_FAILURE_RETRY(writev(output_write.get(), iovs, arraysize(iovs))); if (rc == -1) { fatal_errno("failed to write crash info"); @@ -489,29 +497,19 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c // check to allow all si_code values in calls coming from inside the house. } - void* abort_message = nullptr; - const gwp_asan::AllocatorState* gwp_asan_state = nullptr; - const gwp_asan::AllocationMetadata* gwp_asan_metadata = nullptr; + debugger_process_info process_info = {}; uintptr_t si_val = reinterpret_cast(info->si_ptr); if (signal_number == BIONIC_SIGNAL_DEBUGGER) { if (info->si_code == SI_QUEUE && info->si_pid == __getpid()) { // Allow for the abort message to be explicitly specified via the sigqueue value. // Keep the bottom bit intact for representing whether we want a backtrace or a tombstone. if (si_val != kDebuggerdFallbackSivalUintptrRequestDump) { - abort_message = reinterpret_cast(si_val & ~1); + process_info.abort_msg = reinterpret_cast(si_val & ~1); info->si_ptr = reinterpret_cast(si_val & 1); } } - } else { - if (g_callbacks.get_abort_message) { - abort_message = g_callbacks.get_abort_message(); - } - if (g_callbacks.get_gwp_asan_state) { - gwp_asan_state = g_callbacks.get_gwp_asan_state(); - } - if (g_callbacks.get_gwp_asan_metadata) { - gwp_asan_metadata = g_callbacks.get_gwp_asan_metadata(); - } + } else if (g_callbacks.get_process_info) { + process_info = g_callbacks.get_process_info(); } // If sival_int is ~0, it means that the fallback handler has been called @@ -524,7 +522,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c // This check might be racy if another thread sets NO_NEW_PRIVS, but this should be unlikely, // you can only set NO_NEW_PRIVS to 1, and the effect should be at worst a single missing // ANR trace. - debuggerd_fallback_handler(info, static_cast(context), abort_message); + debuggerd_fallback_handler(info, static_cast(context), process_info.abort_msg); resend_signal(info); return; } @@ -543,10 +541,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c .pseudothread_tid = -1, .siginfo = info, .ucontext = context, - .abort_msg = reinterpret_cast(abort_message), - .fdsan_table = reinterpret_cast(android_fdsan_get_fd_table()), - .gwp_asan_state = reinterpret_cast(gwp_asan_state), - .gwp_asan_metadata = reinterpret_cast(gwp_asan_metadata), + .process_info = process_info, }; // Set PR_SET_DUMPABLE to 1, so that crash_dump can ptrace us. diff --git a/debuggerd/include/debuggerd/handler.h b/debuggerd/include/debuggerd/handler.h index 665d24a7f..665029468 100644 --- a/debuggerd/include/debuggerd/handler.h +++ b/debuggerd/include/debuggerd/handler.h @@ -33,13 +33,20 @@ struct AllocatorState; struct AllocationMetadata; }; // namespace gwp_asan +// When updating this data structure, CrashInfoDataDynamic and the code in +// ReadCrashInfo() must also be updated. +struct debugger_process_info { + void* abort_msg; + void* fdsan_table; + const gwp_asan::AllocatorState* gwp_asan_state; + const gwp_asan::AllocationMetadata* gwp_asan_metadata; +}; + // These callbacks are called in a signal handler, and thus must be async signal safe. // If null, the callbacks will not be called. typedef struct { - struct abort_msg_t* (*get_abort_message)(); + debugger_process_info (*get_process_info)(); void (*post_dump)(); - const struct gwp_asan::AllocatorState* (*get_gwp_asan_state)(); - const struct gwp_asan::AllocationMetadata* (*get_gwp_asan_metadata)(); } debuggerd_callbacks_t; void debuggerd_init(debuggerd_callbacks_t* callbacks); diff --git a/debuggerd/protocol.h b/debuggerd/protocol.h index bf5386412..e85660c85 100644 --- a/debuggerd/protocol.h +++ b/debuggerd/protocol.h @@ -85,17 +85,14 @@ struct __attribute__((__packed__)) CrashInfoHeader { uint32_t version; }; -struct __attribute__((__packed__)) CrashInfoDataV1 { +struct __attribute__((__packed__)) CrashInfoDataStatic { siginfo_t siginfo; ucontext_t ucontext; uintptr_t abort_msg_address; }; -struct __attribute__((__packed__)) CrashInfoDataV2 : public CrashInfoDataV1 { +struct __attribute__((__packed__)) CrashInfoDataDynamic : public CrashInfoDataStatic { uintptr_t fdsan_table_address; -}; - -struct __attribute__((__packed__)) CrashInfoDataV3 : public CrashInfoDataV2 { uintptr_t gwp_asan_state; uintptr_t gwp_asan_metadata; }; @@ -103,8 +100,7 @@ struct __attribute__((__packed__)) CrashInfoDataV3 : public CrashInfoDataV2 { struct __attribute__((__packed__)) CrashInfo { CrashInfoHeader header; union { - CrashInfoDataV1 v1; - CrashInfoDataV2 v2; - CrashInfoDataV3 v3; + CrashInfoDataStatic s; + CrashInfoDataDynamic d; } data; };