From 9da1f51c102e18ef88b7f244b2c871054e1dd9c8 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 6 Aug 2018 15:38:29 -0700 Subject: [PATCH 1/4] crash_dump: pass the address of the fdsan table. Pass the address of the fdsan table down to crash_dump so that we can dump the fdsan table along with the open file descriptor list. Test: debuggerd_test Test: manually ran an old static_crasher Change-Id: Icbac5487109f2db1e1061c4d46de11b016b299e3 --- debuggerd/Android.bp | 2 +- debuggerd/crash_dump.cpp | 58 ++++++++++++++++++------- debuggerd/handler/debuggerd_handler.cpp | 15 ++++--- debuggerd/protocol.h | 19 +++++++- 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index 1b20157b3..385f3850a 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -43,7 +43,7 @@ cc_library_shared { export_include_dirs: ["tombstoned/include"], } -// Utility library to tombstoned and get an output fd. +// Utility library to talk to tombstoned and get an output fd. cc_library_static { name: "libtombstoned_client_static", defaults: ["debuggerd_defaults"], diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 40cf6c340..43b7e05fc 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -249,24 +249,48 @@ static void ParseArgs(int argc, char** argv, pid_t* pseudothread_tid, DebuggerdD } static void ReadCrashInfo(unique_fd& fd, siginfo_t* siginfo, - std::unique_ptr* regs, uintptr_t* abort_address) { + std::unique_ptr* regs, uintptr_t* abort_msg_address, + uintptr_t* fdsan_table_address) { std::aligned_storage::type buf; + CrashInfo* crash_info = reinterpret_cast(&buf); ssize_t rc = TEMP_FAILURE_RETRY(read(fd.get(), &buf, sizeof(buf))); if (rc == -1) { PLOG(FATAL) << "failed to read target ucontext"; - } else if (rc != sizeof(CrashInfo)) { - LOG(FATAL) << "read " << rc << " bytes when reading target crash information, expected " - << sizeof(CrashInfo); + } else { + 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); + break; + + default: + LOG(FATAL) << "unexpected CrashInfo version: " << crash_info->header.version; + break; + }; + + if (rc != expected_size) { + LOG(FATAL) << "read " << rc << " bytes when reading target crash information, expected " + << expected_size; + } } - CrashInfo* crash_info = reinterpret_cast(&buf); - if (crash_info->version != 1) { - LOG(FATAL) << "version mismatch, expected 1, received " << crash_info->version; - } + *fdsan_table_address = 0; + switch (crash_info->header.version) { + case 2: + *fdsan_table_address = crash_info->data.v2.fdsan_table_address; + case 1: + *abort_msg_address = crash_info->data.v1.abort_msg_address; + *siginfo = crash_info->data.v1.siginfo; + regs->reset(Regs::CreateFromUcontext(Regs::CurrentArch(), &crash_info->data.v1.ucontext)); + break; - *siginfo = crash_info->siginfo; - regs->reset(Regs::CreateFromUcontext(Regs::CurrentArch(), &crash_info->ucontext)); - *abort_address = crash_info->abort_msg_address; + default: + __builtin_unreachable(); + } } // Wait for a process to clone and return the child's pid. @@ -369,7 +393,8 @@ int main(int argc, char** argv) { ATRACE_NAME("after reparent"); pid_t pseudothread_tid; DebuggerdDumpType dump_type; - uintptr_t abort_address = 0; + uintptr_t abort_msg_address = 0; + uintptr_t fdsan_table_address = 0; Initialize(argv); ParseArgs(argc, argv, &pseudothread_tid, &dump_type); @@ -429,7 +454,8 @@ int main(int argc, char** argv) { if (thread == g_target_thread) { // Read the thread's registers along with the rest of the crash info out of the pipe. - ReadCrashInfo(input_pipe, &siginfo, &info.registers, &abort_address); + ReadCrashInfo(input_pipe, &siginfo, &info.registers, &abort_msg_address, + &fdsan_table_address); info.siginfo = &siginfo; info.signo = info.siginfo->si_signo; } else { @@ -504,8 +530,8 @@ int main(int argc, char** argv) { g_output_fd = std::move(devnull); } - LOG(INFO) << "performing dump of process " << target_process << " (target tid = " << g_target_thread - << ")"; + LOG(INFO) << "performing dump of process " << target_process + << " (target tid = " << g_target_thread << ")"; int signo = siginfo.si_signo; bool fatal_signal = signo != DEBUGGER_SIGNAL; @@ -543,7 +569,7 @@ int main(int argc, char** argv) { } else { ATRACE_NAME("engrave_tombstone"); engrave_tombstone(std::move(g_output_fd), map.get(), process_memory.get(), thread_info, - g_target_thread, abort_address, &open_files, &amfd_data); + g_target_thread, abort_msg_address, &open_files, &amfd_data); } if (fatal_signal) { diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index 615fb46ad..91e6f7111 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -108,6 +108,7 @@ class ErrnoRestorer { int saved_errno_; }; +extern "C" void* android_fdsan_get_fd_table(); extern "C" void debuggerd_fallback_handler(siginfo_t*, ucontext_t*, void*); static debuggerd_callbacks_t g_callbacks; @@ -286,6 +287,7 @@ struct debugger_thread_info { siginfo_t* siginfo; void* ucontext; uintptr_t abort_msg; + uintptr_t fdsan_table; }; // Logging and contacting debuggerd requires free file descriptors, which we might not have. @@ -330,23 +332,23 @@ static int debuggerd_dispatch_pseudothread(void* arg) { } // ucontext_t is absurdly large on AArch64, so piece it together manually with writev. - uint32_t version = 1; - constexpr size_t expected = - sizeof(version) + sizeof(siginfo_t) + sizeof(ucontext_t) + sizeof(uintptr_t); + uint32_t version = 2; + constexpr size_t expected = sizeof(CrashInfoHeader) + sizeof(CrashInfoDataV2); errno = 0; if (fcntl(output_write.get(), F_SETPIPE_SZ, expected) < static_cast(expected)) { - fatal_errno("failed to set pipe bufer size"); + fatal_errno("failed to set pipe buffer size"); } - struct iovec iovs[4] = { + struct iovec iovs[5] = { {.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)}, }; - ssize_t rc = TEMP_FAILURE_RETRY(writev(output_write.get(), iovs, 4)); + ssize_t rc = TEMP_FAILURE_RETRY(writev(output_write.get(), iovs, 5)); if (rc == -1) { fatal_errno("failed to write crash info"); } else if (rc != expected) { @@ -504,6 +506,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c .siginfo = info, .ucontext = context, .abort_msg = reinterpret_cast(abort_message), + .fdsan_table = reinterpret_cast(android_fdsan_get_fd_table()), }; // Set PR_SET_DUMPABLE to 1, so that crash_dump can ptrace us. diff --git a/debuggerd/protocol.h b/debuggerd/protocol.h index 6903b0e55..bfd0fbbe2 100644 --- a/debuggerd/protocol.h +++ b/debuggerd/protocol.h @@ -81,9 +81,24 @@ struct InterceptResponse { }; // Sent from handler to crash_dump via pipe. -struct __attribute__((__packed__)) CrashInfo { - uint32_t version; // must be 1. +struct __attribute__((__packed__)) CrashInfoHeader { + uint32_t version; +}; + +struct __attribute__((__packed__)) CrashInfoDataV1 { siginfo_t siginfo; ucontext_t ucontext; uintptr_t abort_msg_address; }; + +struct __attribute__((__packed__)) CrashInfoDataV2 : public CrashInfoDataV1 { + uintptr_t fdsan_table_address; +}; + +struct __attribute__((__packed__)) CrashInfo { + CrashInfoHeader header; + union { + CrashInfoDataV1 v1; + CrashInfoDataV2 v2; + } data; +}; From ce841d91fb80f2f87aa43a82b8800704d324a99c Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 6 Aug 2018 18:26:42 -0700 Subject: [PATCH 2/4] libdebuggerd: extract and print the fdsan table. This commit only prints the raw value of the owner tag, pretty-printing will come in a follow-up commit. Test: debuggerd `pidof adbd` Test: static_crasher fdsan_file + manual inspection of tombstone Change-Id: Idb7375a12e410d5b51e6fcb6885d4beb20bccd0e --- debuggerd/Android.bp | 3 + debuggerd/crash_dump.cpp | 15 +++- .../include/libdebuggerd/open_files_list.h | 26 +++--- debuggerd/libdebuggerd/open_files_list.cpp | 79 +++++++++++++++++-- .../test/open_files_list_test.cpp | 6 +- 5 files changed, 108 insertions(+), 21 deletions(-) diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index 385f3850a..03b328727 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -166,6 +166,9 @@ cc_library_static { local_include_dirs: ["libdebuggerd/include"], export_include_dirs: ["libdebuggerd/include"], + // Needed for private/bionic_fdsan.h + include_dirs: ["bionic/libc"], + static_libs: [ "libbacktrace", "libunwindstack", diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 43b7e05fc..93f757236 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -412,7 +412,7 @@ int main(int argc, char** argv) { OpenFilesList open_files; { ATRACE_NAME("open files"); - populate_open_files_list(g_target_thread, &open_files); + populate_open_files_list(&open_files, g_target_thread); } // In order to reduce the duration that we pause the process for, we ptrace @@ -567,9 +567,16 @@ int main(int argc, char** argv) { ATRACE_NAME("dump_backtrace"); dump_backtrace(std::move(g_output_fd), map.get(), thread_info, g_target_thread); } else { - ATRACE_NAME("engrave_tombstone"); - engrave_tombstone(std::move(g_output_fd), map.get(), process_memory.get(), thread_info, - g_target_thread, abort_msg_address, &open_files, &amfd_data); + { + ATRACE_NAME("fdsan table dump"); + populate_fdsan_table(&open_files, process_memory, fdsan_table_address); + } + + { + ATRACE_NAME("engrave_tombstone"); + engrave_tombstone(std::move(g_output_fd), map.get(), process_memory.get(), thread_info, + g_target_thread, abort_msg_address, &open_files, &amfd_data); + } } if (fatal_signal) { diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/open_files_list.h b/debuggerd/libdebuggerd/include/libdebuggerd/open_files_list.h index 4727ca4d7..d47f2ddf6 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/open_files_list.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/open_files_list.h @@ -14,23 +14,31 @@ * limitations under the License. */ -#ifndef _DEBUGGERD_OPEN_FILES_LIST_H -#define _DEBUGGERD_OPEN_FILES_LIST_H +#pragma once +#include #include +#include +#include #include #include -#include #include "utility.h" -typedef std::vector> OpenFilesList; +struct FDInfo { + std::optional path; + std::optional fdsan_owner; +}; -/* Populates the given list with open files for the given process. */ -void populate_open_files_list(pid_t pid, OpenFilesList* list); +using OpenFilesList = std::map; -/* Dumps the open files list to the log. */ +// Populates the given list with open files for the given process. +void populate_open_files_list(OpenFilesList* list, pid_t pid); + +// Populates the given list with the target process's fdsan table. +void populate_fdsan_table(OpenFilesList* list, std::shared_ptr memory, + uint64_t fdsan_table_address); + +// Dumps the open files list to the log. void dump_open_files_list(log_t* log, const OpenFilesList& files, const char* prefix); - -#endif // _DEBUGGERD_OPEN_FILES_LIST_H diff --git a/debuggerd/libdebuggerd/open_files_list.cpp b/debuggerd/libdebuggerd/open_files_list.cpp index b12703e14..1fdf2369a 100644 --- a/debuggerd/libdebuggerd/open_files_list.cpp +++ b/debuggerd/libdebuggerd/open_files_list.cpp @@ -32,10 +32,12 @@ #include #include +#include #include "libdebuggerd/utility.h" +#include "private/bionic_fdsan.h" -void populate_open_files_list(pid_t pid, OpenFilesList* list) { +void populate_open_files_list(OpenFilesList* list, pid_t pid) { std::string fd_dir_name = "/proc/" + std::to_string(pid) + "/fd"; std::unique_ptr dir(opendir(fd_dir_name.c_str()), closedir); if (dir == nullptr) { @@ -53,17 +55,84 @@ void populate_open_files_list(pid_t pid, OpenFilesList* list) { std::string path = fd_dir_name + "/" + std::string(de->d_name); std::string target; if (android::base::Readlink(path, &target)) { - list->emplace_back(fd, target); + (*list)[fd].path = target; } else { + (*list)[fd].path = "???"; ALOGE("failed to readlink %s: %s", path.c_str(), strerror(errno)); - list->emplace_back(fd, "???"); } } } +void populate_fdsan_table(OpenFilesList* list, std::shared_ptr memory, + uint64_t fdsan_table_address) { + constexpr size_t inline_fds = sizeof(FdTable::entries) / sizeof(*FdTable::entries); + static_assert(inline_fds == 128); + size_t entry_offset = offsetof(FdTable, entries); + for (size_t i = 0; i < inline_fds; ++i) { + uint64_t address = fdsan_table_address + entry_offset + sizeof(FdEntry) * i; + FdEntry entry; + if (!memory->Read(address, &entry, sizeof(entry))) { + ALOGE("failed to read fdsan table entry %zu: %s", i, strerror(errno)); + return; + } + ALOGE("fd %zu = %#" PRIx64, i, entry.close_tag.load()); + if (entry.close_tag) { + (*list)[i].fdsan_owner = entry.close_tag.load(); + } + } + + size_t overflow_offset = offsetof(FdTable, overflow); + uintptr_t overflow = 0; + if (!memory->Read(fdsan_table_address + overflow_offset, &overflow, sizeof(overflow))) { + ALOGE("failed to read fdsan table overflow pointer: %s", strerror(errno)); + return; + } + + if (!overflow) { + return; + } + + size_t overflow_length; + if (!memory->Read(overflow, &overflow_length, sizeof(overflow_length))) { + ALOGE("failed to read fdsan overflow table length: %s", strerror(errno)); + return; + } + + if (overflow_length > 131072) { + ALOGE("unreasonable large fdsan overflow table size %zu, bailing out", overflow_length); + return; + } + + for (size_t i = 0; i < overflow_length; ++i) { + int fd = i + inline_fds; + uint64_t address = overflow + offsetof(FdTableOverflow, entries) + i * sizeof(FdEntry); + FdEntry entry; + if (!memory->Read(address, &entry, sizeof(entry))) { + ALOGE("failed to read fdsan overflow entry for fd %d: %s", fd, strerror(errno)); + return; + } + if (entry.close_tag) { + (*list)[fd].fdsan_owner = entry.close_tag; + } + } + return; +} + void dump_open_files_list(log_t* log, const OpenFilesList& files, const char* prefix) { - for (auto& file : files) { - _LOG(log, logtype::OPEN_FILES, "%sfd %i: %s\n", prefix, file.first, file.second.c_str()); + for (auto& [fd, entry] : files) { + const std::optional& path = entry.path; + const std::optional& fdsan_owner = entry.fdsan_owner; + if (path && fdsan_owner) { + _LOG(log, logtype::OPEN_FILES, "%sfd %i: %s (owned by %#" PRIx64 ")\n", prefix, fd, + path->c_str(), *fdsan_owner); + } else if (path && !fdsan_owner) { + _LOG(log, logtype::OPEN_FILES, "%sfd %i: %s (unowned)\n", prefix, fd, path->c_str()); + } else if (!path && fdsan_owner) { + _LOG(log, logtype::OPEN_FILES, "%sfd %i: (owned by %#" PRIx64 ")\n", prefix, fd, + *fdsan_owner); + } else { + ALOGE("OpenFilesList contains an entry (fd %d) with no path or owner", fd); + } } } diff --git a/debuggerd/libdebuggerd/test/open_files_list_test.cpp b/debuggerd/libdebuggerd/test/open_files_list_test.cpp index acac72c22..d7036fd39 100644 --- a/debuggerd/libdebuggerd/test/open_files_list_test.cpp +++ b/debuggerd/libdebuggerd/test/open_files_list_test.cpp @@ -34,13 +34,13 @@ TEST(OpenFilesListTest, BasicTest) { // Get the list of open files for this process. OpenFilesList list; - populate_open_files_list(getpid(), &list); + populate_open_files_list(&list, getpid()); // Verify our open file is in the list. bool found = false; - for (auto& file : list) { + for (auto& file : list) { if (file.first == tf.fd) { - EXPECT_EQ(file.second, std::string(tf.path)); + EXPECT_EQ(file.second.path.value_or(""), std::string(tf.path)); found = true; break; } From 9d100f1043b629373f28b85de12c4e9b80d270bf Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 6 Aug 2018 18:36:58 -0700 Subject: [PATCH 3/4] adb: move AdbCloser to its rightful place. Test: mma Change-Id: Ie74c49e8abf72f594a35d04b2b0d99b96f58f8d0 --- adb/adb_unique_fd.cpp | 4 ++++ adb/adb_utils.cpp | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/adb/adb_unique_fd.cpp b/adb/adb_unique_fd.cpp index 2079be152..58e768e13 100644 --- a/adb/adb_unique_fd.cpp +++ b/adb/adb_unique_fd.cpp @@ -21,6 +21,10 @@ #include "sysdeps.h" +void AdbCloser::Close(int fd) { + adb_close(fd); +} + #if !defined(_WIN32) bool Pipe(unique_fd* read, unique_fd* write, int flags) { int pipefd[2]; diff --git a/adb/adb_utils.cpp b/adb/adb_utils.cpp index 0c3327fc3..ffac315b3 100644 --- a/adb/adb_utils.cpp +++ b/adb/adb_utils.cpp @@ -274,10 +274,6 @@ std::string adb_get_android_dir_path() { return android_dir; } -void AdbCloser::Close(int fd) { - adb_close(fd); -} - int syntax_error(const char* fmt, ...) { fprintf(stderr, "adb: usage: "); From 297d9bf8ea41e6eb8b50000163a13d30f6478c7e Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 6 Aug 2018 18:38:47 -0700 Subject: [PATCH 4/4] adb: actually enable fdsan. adb was using a custom unique_fd closer that didn't have an implementation for fdsan, which meant that none of our FDs were actually tracked. Guard this behind ifdefs so that we only use this on Windows, and delete our implementation of Pipe in favor of the one in libbase while we're at it. libbase's implementation always sets O_CLOEXEC, so fix up the instance of Pipe that doesn't expect that. Test: mma Test: adb start-server Test: debuggerd `pidof adbd` Change-Id: Ic29d641a2f93fb42384b00c51775048c8bcbe152 --- adb/adb.cpp | 4 ++++ adb/adb_unique_fd.cpp | 43 +------------------------------------------ adb/adb_unique_fd.h | 6 +++--- 3 files changed, 8 insertions(+), 45 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 38c6f62c9..a8fe73651 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -891,6 +891,10 @@ int launch_server(const std::string& socket_spec) { // child side of the fork pipe_read.reset(); + // android::base::Pipe unconditionally opens the pipe with O_CLOEXEC. + // Undo this manually. + fcntl(pipe_write.get(), F_SETFD, 0); + char reply_fd[30]; snprintf(reply_fd, sizeof(reply_fd), "%d", pipe_write.get()); // child process diff --git a/adb/adb_unique_fd.cpp b/adb/adb_unique_fd.cpp index 58e768e13..dec73bc2e 100644 --- a/adb/adb_unique_fd.cpp +++ b/adb/adb_unique_fd.cpp @@ -21,49 +21,8 @@ #include "sysdeps.h" +#if defined(_WIN32) void AdbCloser::Close(int fd) { adb_close(fd); } - -#if !defined(_WIN32) -bool Pipe(unique_fd* read, unique_fd* write, int flags) { - int pipefd[2]; -#if !defined(__APPLE__) - if (pipe2(pipefd, flags) != 0) { - return false; - } -#else - // Darwin doesn't have pipe2. Implement it ourselves. - if (flags != 0 && (flags & ~(O_CLOEXEC | O_NONBLOCK)) != 0) { - errno = EINVAL; - return false; - } - - if (pipe(pipefd) != 0) { - return false; - } - - if (flags & O_CLOEXEC) { - if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) != 0 || - fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) != 0) { - adb_close(pipefd[0]); - adb_close(pipefd[1]); - return false; - } - } - - if (flags & O_NONBLOCK) { - if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) != 0 || - fcntl(pipefd[1], F_SETFL, O_NONBLOCK) != 0) { - adb_close(pipefd[0]); - adb_close(pipefd[1]); - return false; - } - } -#endif - - read->reset(pipefd[0]); - write->reset(pipefd[1]); - return true; -} #endif diff --git a/adb/adb_unique_fd.h b/adb/adb_unique_fd.h index bad501abb..d47213d75 100644 --- a/adb/adb_unique_fd.h +++ b/adb/adb_unique_fd.h @@ -21,15 +21,15 @@ #include +#if defined(_WIN32) // Helper to automatically close an FD when it goes out of scope. struct AdbCloser { static void Close(int fd); }; using unique_fd = android::base::unique_fd_impl; - -#if !defined(_WIN32) -bool Pipe(unique_fd* read, unique_fd* write, int flags = 0); +#else +using unique_fd = android::base::unique_fd; #endif template