From a50f61f8fa903117a6df82d164628de310f16ae9 Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:40:51 +0000 Subject: [PATCH 1/5] Revert "Fix build breakage." This reverts commit 675cb30f05907f6c509815365c9af58a8671fe4e. Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Change-Id: I82d228f2bc3e6b426d4703732e1c8766815ccc97 --- debuggerd/libdebuggerd/tombstone_proto.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp index 801b11242..17e401e25 100644 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ b/debuggerd/libdebuggerd/tombstone_proto.cpp @@ -447,7 +447,7 @@ void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::Unwinder* unwind if (process_info.has_fault_address) { sig.set_has_fault_address(true); - sig.set_fault_address(process_info.untagged_fault_address); + sig.set_fault_address(process_info.fault_address); } *result.mutable_signal_info() = sig; From 1e45d3f2239333217d3252f78151f4294fda4e80 Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:36:12 +0000 Subject: [PATCH 2/5] Revert "libdebuggerd: add protobuf implementation." Revert "Let crash_dump read /proc/$PID." Revert submission 1556807-tombstone_proto Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Reverted Changes: Ide6811297:tombstoned: switch from goto to RAII. I8d285c4b4:tombstoned: make it easier to add more types of ou... Id0f0fa285:tombstoned: support for protobuf fds. I6be6082ab:Let crash_dump read /proc/$PID. Id812ca390:Make protobuf vendor_ramdisk_available. Ieeece6e6d:libdebuggerd: add protobuf implementation. Change-Id: Ia0a1ee57e7630e01c495dc166218f665340aad7f --- debuggerd/Android.bp | 21 - debuggerd/crash_dump.cpp | 28 +- debuggerd/handler/debuggerd_fallback.cpp | 19 +- debuggerd/handler/debuggerd_handler.cpp | 2 +- .../include/libdebuggerd/tombstone.h | 17 +- .../libdebuggerd/include/libdebuggerd/types.h | 2 - .../include/libdebuggerd/utility.h | 2 - debuggerd/libdebuggerd/tombstone.cpp | 95 ++-- debuggerd/libdebuggerd/tombstone_proto.cpp | 478 ------------------ .../libdebuggerd/tombstone_proto_to_text.cpp | 361 ------------- debuggerd/libdebuggerd/utility.cpp | 54 +- debuggerd/pbtombstone.cpp | 62 --- debuggerd/proto/Android.bp | 28 - debuggerd/proto/tombstone.proto | 118 ----- .../seccomp_policy/crash_dump.arm.policy | 1 - .../seccomp_policy/crash_dump.arm64.policy | 1 - .../seccomp_policy/crash_dump.policy.def | 1 - .../seccomp_policy/crash_dump.x86.policy | 1 - .../seccomp_policy/crash_dump.x86_64.policy | 1 - 19 files changed, 78 insertions(+), 1214 deletions(-) delete mode 100644 debuggerd/libdebuggerd/tombstone_proto.cpp delete mode 100644 debuggerd/libdebuggerd/tombstone_proto_to_text.cpp delete mode 100644 debuggerd/pbtombstone.cpp delete mode 100644 debuggerd/proto/Android.bp delete mode 100644 debuggerd/proto/tombstone.proto diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index 2390d7979..fd6239253 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -176,8 +176,6 @@ cc_library_static { "libdebuggerd/open_files_list.cpp", "libdebuggerd/scudo.cpp", "libdebuggerd/tombstone.cpp", - "libdebuggerd/tombstone_proto.cpp", - "libdebuggerd/tombstone_proto_to_text.cpp", "libdebuggerd/utility.cpp", ], @@ -208,8 +206,6 @@ cc_library_static { whole_static_libs: [ "gwp_asan_crash_handler", "libscudo", - "libtombstone_proto", - "libprotobuf-cpp-lite", ], target: { @@ -232,20 +228,6 @@ cc_library_static { }, } -cc_binary { - name: "pbtombstone", - defaults: ["debuggerd_defaults"], - srcs: ["pbtombstone.cpp"], - static_libs: [ - "libbase", - "libdebuggerd", - "liblog", - "libprotobuf-cpp-lite", - "libtombstone_proto", - "libunwindstack", - ], -} - cc_test { name: "debuggerd_test", defaults: ["debuggerd_defaults"], @@ -350,9 +332,6 @@ cc_binary { "libtombstoned_client_static", "libdebuggerd", "libcutils", - - "libtombstone_proto", - "libprotobuf-cpp-lite", ], shared_libs: [ diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index 51afcc280..007a20fce 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -195,7 +195,6 @@ static pid_t g_target_thread = -1; static bool g_tombstoned_connected = false; static unique_fd g_tombstoned_socket; static unique_fd g_output_fd; -static unique_fd g_proto_fd; static void DefuseSignalHandlers() { // Don't try to dump ourselves. @@ -216,7 +215,7 @@ static void Initialize(char** argv) { // If we abort before we get an output fd, contact tombstoned to let any // potential listeners know that we failed. if (!g_tombstoned_connected) { - if (!tombstoned_connect(g_target_thread, &g_tombstoned_socket, &g_output_fd, &g_proto_fd, + if (!tombstoned_connect(g_target_thread, &g_tombstoned_socket, &g_output_fd, kDebuggerdAnyIntercept)) { // We failed to connect, not much we can do. LOG(ERROR) << "failed to connected to tombstoned to report failure"; @@ -249,20 +248,10 @@ static void ParseArgs(int argc, char** argv, pid_t* pseudothread_tid, DebuggerdD } int dump_type_int; - if (!android::base::ParseInt(argv[3], &dump_type_int, 0)) { + if (!android::base::ParseInt(argv[3], &dump_type_int, 0, 1)) { LOG(FATAL) << "invalid requested dump type: " << argv[3]; } - *dump_type = static_cast(dump_type_int); - switch (*dump_type) { - case kDebuggerdNativeBacktrace: - case kDebuggerdTombstone: - case kDebuggerdTombstoneProto: - break; - - default: - LOG(FATAL) << "invalid requested dump type: " << dump_type_int; - } } static void ReadCrashInfo(unique_fd& fd, siginfo_t* siginfo, @@ -491,11 +480,6 @@ int main(int argc, char** argv) { info.process_name = process_name; info.thread_name = get_thread_name(thread); - unique_fd attr_fd(openat(target_proc_fd, "attr/current", O_RDONLY | O_CLOEXEC)); - if (!android::base::ReadFdToString(attr_fd, &info.selinux_label)) { - PLOG(WARNING) << "failed to read selinux label"; - } - if (!ptrace_interrupt(thread, &info.signo)) { PLOG(WARNING) << "failed to ptrace interrupt thread " << thread; ptrace(PTRACE_DETACH, thread, 0, 0); @@ -574,8 +558,8 @@ int main(int argc, char** argv) { { ATRACE_NAME("tombstoned_connect"); LOG(INFO) << "obtaining output fd from tombstoned, type: " << dump_type; - g_tombstoned_connected = tombstoned_connect(g_target_thread, &g_tombstoned_socket, &g_output_fd, - &g_proto_fd, dump_type); + g_tombstoned_connected = + tombstoned_connect(g_target_thread, &g_tombstoned_socket, &g_output_fd, dump_type); } if (g_tombstoned_connected) { @@ -628,8 +612,8 @@ int main(int argc, char** argv) { { ATRACE_NAME("engrave_tombstone"); - engrave_tombstone(std::move(g_output_fd), std::move(g_proto_fd), &unwinder, thread_info, - g_target_thread, process_info, &open_files, &amfd_data); + engrave_tombstone(std::move(g_output_fd), &unwinder, thread_info, g_target_thread, process_info, + &open_files, &amfd_data); } } diff --git a/debuggerd/handler/debuggerd_fallback.cpp b/debuggerd/handler/debuggerd_fallback.cpp index feafa7377..e103c82f4 100644 --- a/debuggerd/handler/debuggerd_fallback.cpp +++ b/debuggerd/handler/debuggerd_fallback.cpp @@ -92,15 +92,15 @@ static void debuggerd_fallback_trace(int output_fd, ucontext_t* ucontext) { __linker_disable_fallback_allocator(); } -static void debuggerd_fallback_tombstone(int output_fd, int proto_fd, ucontext_t* ucontext, - siginfo_t* siginfo, void* abort_message) { +static void debuggerd_fallback_tombstone(int output_fd, ucontext_t* ucontext, siginfo_t* siginfo, + void* abort_message) { if (!__linker_enable_fallback_allocator()) { async_safe_format_log(ANDROID_LOG_ERROR, "libc", "fallback allocator already in use"); return; } - engrave_tombstone_ucontext(output_fd, proto_fd, reinterpret_cast(abort_message), - siginfo, ucontext); + engrave_tombstone_ucontext(output_fd, reinterpret_cast(abort_message), siginfo, + ucontext); __linker_disable_fallback_allocator(); } @@ -232,8 +232,7 @@ static void trace_handler(siginfo_t* info, ucontext_t* ucontext) { // Fetch output fd from tombstoned. unique_fd tombstone_socket, output_fd; - if (!tombstoned_connect(getpid(), &tombstone_socket, &output_fd, nullptr, - kDebuggerdNativeBacktrace)) { + if (!tombstoned_connect(getpid(), &tombstone_socket, &output_fd, kDebuggerdNativeBacktrace)) { async_safe_format_log(ANDROID_LOG_ERROR, "libc", "missing crash_dump_fallback() in selinux policy?"); goto exit; @@ -326,10 +325,10 @@ static void crash_handler(siginfo_t* info, ucontext_t* ucontext, void* abort_mes _exit(1); } - unique_fd tombstone_socket, output_fd, proto_fd; - bool tombstoned_connected = tombstoned_connect(getpid(), &tombstone_socket, &output_fd, &proto_fd, - kDebuggerdTombstoneProto); - debuggerd_fallback_tombstone(output_fd.get(), proto_fd.get(), ucontext, info, abort_message); + unique_fd tombstone_socket, output_fd; + bool tombstoned_connected = + tombstoned_connect(getpid(), &tombstone_socket, &output_fd, kDebuggerdTombstone); + debuggerd_fallback_tombstone(output_fd.get(), ucontext, info, abort_message); if (tombstoned_connected) { tombstoned_notify_completion(tombstone_socket.get()); } diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index ca809e44f..1297c4d33 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -312,7 +312,7 @@ static DebuggerdDumpType get_dump_type(const debugger_thread_info* thread_info) return kDebuggerdNativeBacktrace; } - return kDebuggerdTombstoneProto; + return kDebuggerdTombstone; } static int debuggerd_dispatch_pseudothread(void* arg) { diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h b/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h index bf2cbb3fb..3ff7d629d 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/tombstone.h @@ -21,7 +21,6 @@ #include #include -#include #include #include @@ -31,8 +30,6 @@ #include "types.h" // Forward declarations -class Tombstone; - namespace unwindstack { class Unwinder; } @@ -47,21 +44,13 @@ constexpr size_t kMaxFrames = 256; int open_tombstone(std::string* path); /* Creates a tombstone file and writes the crash dump to it. */ -void engrave_tombstone(android::base::unique_fd output_fd, android::base::unique_fd proto_fd, - unwindstack::Unwinder* unwinder, +void engrave_tombstone(android::base::unique_fd output_fd, unwindstack::Unwinder* unwinder, const std::map& thread_info, pid_t target_thread, const ProcessInfo& process_info, OpenFilesList* open_files, std::string* amfd_data); -void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_msg_address, - siginfo_t* siginfo, ucontext_t* ucontext); +void engrave_tombstone_ucontext(int tombstone_fd, uint64_t abort_msg_address, siginfo_t* siginfo, + ucontext_t* ucontext); -void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::Unwinder* unwinder, - const std::map& threads, pid_t target_thread, - const ProcessInfo& process_info, const OpenFilesList* open_files); - -bool tombstone_proto_to_text( - const Tombstone& tombstone, - std::function callback); #endif // _DEBUGGERD_TOMBSTONE_H diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/types.h b/debuggerd/libdebuggerd/include/libdebuggerd/types.h index d5b07355f..86522ee36 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/types.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/types.h @@ -31,9 +31,7 @@ struct ThreadInfo { std::string thread_name; pid_t pid; - std::string process_name; - std::string selinux_label; int signo = 0; siginfo_t* siginfo = nullptr; diff --git a/debuggerd/libdebuggerd/include/libdebuggerd/utility.h b/debuggerd/libdebuggerd/include/libdebuggerd/utility.h index d71b76f7c..29fb9a4f6 100644 --- a/debuggerd/libdebuggerd/include/libdebuggerd/utility.h +++ b/debuggerd/libdebuggerd/include/libdebuggerd/utility.h @@ -81,8 +81,6 @@ class Memory; void log_backtrace(log_t* log, unwindstack::Unwinder* unwinder, const char* prefix); -ssize_t dump_memory(void* out, size_t len, size_t* start_offset, uint64_t* addr, - unwindstack::Memory* memory); void dump_memory(log_t* log, unwindstack::Memory* backtrace, uint64_t addr, const std::string&); void drop_capabilities(); diff --git a/debuggerd/libdebuggerd/tombstone.cpp b/debuggerd/libdebuggerd/tombstone.cpp index 822b74aea..4bd71926f 100644 --- a/debuggerd/libdebuggerd/tombstone.cpp +++ b/debuggerd/libdebuggerd/tombstone.cpp @@ -63,8 +63,6 @@ #include "gwp_asan/common.h" #include "gwp_asan/crash_handler.h" -#include "tombstone.pb.h" - using android::base::GetBoolProperty; using android::base::GetProperty; using android::base::StringPrintf; @@ -192,7 +190,8 @@ static void dump_thread_info(log_t* log, const ThreadInfo& thread_info) { static std::string get_addr_string(uint64_t addr) { std::string addr_str; #if defined(__LP64__) - addr_str = StringPrintf("%08x'%08x", static_cast(addr >> 32), + addr_str = StringPrintf("%08x'%08x", + static_cast(addr >> 32), static_cast(addr & 0xffffffff)); #else addr_str = StringPrintf("%08x", static_cast(addr)); @@ -395,7 +394,8 @@ static bool dump_thread(log_t* log, unwindstack::Unwinder* unwinder, const Threa if (primary_thread && gwp_asan_crash_data->CrashIsMine()) { gwp_asan_crash_data->DumpCause(log); } else if (thread_info.siginfo && !(primary_thread && scudo_crash_data->CrashIsMine())) { - dump_probable_cause(log, thread_info.siginfo, unwinder->GetMaps(), thread_info.registers.get()); + dump_probable_cause(log, thread_info.siginfo, unwinder->GetMaps(), + thread_info.registers.get()); } if (primary_thread) { @@ -492,7 +492,8 @@ static void dump_log_file(log_t* log, pid_t pid, const char* filename, unsigned // the tombstone file. if (first) { - _LOG(log, logtype::LOGS, "--------- %slog %s\n", tail ? "tail end of " : "", filename); + _LOG(log, logtype::LOGS, "--------- %slog %s\n", + tail ? "tail end of " : "", filename); first = false; } @@ -553,8 +554,8 @@ static void dump_logs(log_t* log, pid_t pid, unsigned int tail) { dump_log_file(log, pid, "main", tail); } -void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_msg_address, - siginfo_t* siginfo, ucontext_t* ucontext) { +void engrave_tombstone_ucontext(int tombstone_fd, uint64_t abort_msg_address, siginfo_t* siginfo, + ucontext_t* ucontext) { pid_t uid = getuid(); pid_t pid = getpid(); pid_t tid = gettid(); @@ -571,18 +572,14 @@ void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_m std::unique_ptr regs( unwindstack::Regs::CreateFromUcontext(unwindstack::Regs::CurrentArch(), ucontext)); - std::string selinux_label; - android::base::ReadFileToString("/proc/self/attr/current", &selinux_label); - std::map threads; threads[tid] = ThreadInfo{ .registers = std::move(regs), .uid = uid, .tid = tid, - .thread_name = std::move(thread_name), + .thread_name = thread_name.c_str(), .pid = pid, - .process_name = std::move(process_name), - .selinux_label = std::move(selinux_label), + .process_name = process_name.c_str(), .siginfo = siginfo, }; @@ -592,23 +589,17 @@ void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_m } ProcessInfo process_info; - unique_fd attr_fd(open("/proc/self/attr/current", O_RDONLY | O_CLOEXEC)); process_info.abort_msg_address = abort_msg_address; - engrave_tombstone(unique_fd(dup(tombstone_fd)), unique_fd(dup(proto_fd)), &unwinder, threads, tid, - process_info, nullptr, nullptr); + engrave_tombstone(unique_fd(dup(tombstone_fd)), &unwinder, threads, tid, process_info, nullptr, + nullptr); } -void engrave_tombstone(unique_fd output_fd, unique_fd proto_fd, unwindstack::Unwinder* unwinder, +void engrave_tombstone(unique_fd output_fd, unwindstack::Unwinder* unwinder, const std::map& threads, pid_t target_thread, const ProcessInfo& process_info, OpenFilesList* open_files, std::string* amfd_data) { // Don't copy log messages to tombstone unless this is a development device. - Tombstone tombstone; - engrave_tombstone_proto(&tombstone, unwinder, threads, target_thread, process_info, open_files); - - if (!tombstone.SerializeToFileDescriptor(proto_fd.get())) { - PLOG(ERROR) << "Failed to write proto tombstone"; - } + bool want_logs = GetBoolProperty("ro.debuggable", false); log_t log; log.current_tid = target_thread; @@ -616,45 +607,35 @@ void engrave_tombstone(unique_fd output_fd, unique_fd proto_fd, unwindstack::Unw log.tfd = output_fd.get(); log.amfd_data = amfd_data; - bool translate_proto = GetBoolProperty("debug.debuggerd.translate_proto_to_text", false); - if (translate_proto) { - tombstone_proto_to_text(tombstone, [&log](const std::string& line, bool should_log) { - _LOG(&log, should_log ? logtype::HEADER : logtype::LOGS, "%s\n", line.c_str()); - }); - } else { - bool want_logs = GetBoolProperty("ro.debuggable", false); + _LOG(&log, logtype::HEADER, "*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***\n"); + dump_header_info(&log); + _LOG(&log, logtype::HEADER, "Timestamp: %s\n", get_timestamp().c_str()); - _LOG(&log, logtype::HEADER, - "*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***\n"); - dump_header_info(&log); - _LOG(&log, logtype::HEADER, "Timestamp: %s\n", get_timestamp().c_str()); + auto it = threads.find(target_thread); + if (it == threads.end()) { + LOG(FATAL) << "failed to find target thread"; + } - auto it = threads.find(target_thread); - if (it == threads.end()) { - LOG(FATAL) << "failed to find target thread"; + dump_thread(&log, unwinder, it->second, process_info, true); + + if (want_logs) { + dump_logs(&log, it->second.pid, 50); + } + + for (auto& [tid, thread_info] : threads) { + if (tid == target_thread) { + continue; } - dump_thread(&log, unwinder, it->second, process_info, true); + dump_thread(&log, unwinder, thread_info, process_info, false); + } - if (want_logs) { - dump_logs(&log, it->second.pid, 50); - } + if (open_files) { + _LOG(&log, logtype::OPEN_FILES, "\nopen files:\n"); + dump_open_files_list(&log, *open_files, " "); + } - for (auto& [tid, thread_info] : threads) { - if (tid == target_thread) { - continue; - } - - dump_thread(&log, unwinder, thread_info, process_info, false); - } - - if (open_files) { - _LOG(&log, logtype::OPEN_FILES, "\nopen files:\n"); - dump_open_files_list(&log, *open_files, " "); - } - - if (want_logs) { - dump_logs(&log, it->second.pid, 0); - } + if (want_logs) { + dump_logs(&log, it->second.pid, 0); } } diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp deleted file mode 100644 index 17e401e25..000000000 --- a/debuggerd/libdebuggerd/tombstone_proto.cpp +++ /dev/null @@ -1,478 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define LOG_TAG "DEBUG" - -#include "libdebuggerd/tombstone.h" - -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include - -#include -#include -#include -#include -#include - -#include -#include -#include -#include -#include - -#include -#include -#include -#include - -#include "libdebuggerd/open_files_list.h" -#include "libdebuggerd/utility.h" -#include "util.h" - -#include "tombstone.pb.h" - -using android::base::StringPrintf; - -// Use the demangler from libc++. -extern "C" char* __cxa_demangle(const char*, char*, size_t*, int* status); - -static Architecture get_arch() { -#if defined(__arm__) - return Architecture::ARM32; -#elif defined(__aarch64__) - return Architecture::ARM64; -#elif defined(__i386__) - return Architecture::X86; -#elif defined(__x86_64__) - return Architecture::X86_64; -#else -#error Unknown architecture! -#endif -} - -static std::optional get_stack_overflow_cause(uint64_t fault_addr, uint64_t sp, - unwindstack::Maps* maps) { - static constexpr uint64_t kMaxDifferenceBytes = 256; - uint64_t difference; - if (sp >= fault_addr) { - difference = sp - fault_addr; - } else { - difference = fault_addr - sp; - } - if (difference <= kMaxDifferenceBytes) { - // The faulting address is close to the current sp, check if the sp - // indicates a stack overflow. - // On arm, the sp does not get updated when the instruction faults. - // In this case, the sp will still be in a valid map, which is the - // last case below. - // On aarch64, the sp does get updated when the instruction faults. - // In this case, the sp will be in either an invalid map if triggered - // on the main thread, or in a guard map if in another thread, which - // will be the first case or second case from below. - unwindstack::MapInfo* map_info = maps->Find(sp); - if (map_info == nullptr) { - return "stack pointer is in a non-existent map; likely due to stack overflow."; - } else if ((map_info->flags & (PROT_READ | PROT_WRITE)) != (PROT_READ | PROT_WRITE)) { - return "stack pointer is not in a rw map; likely due to stack overflow."; - } else if ((sp - map_info->start) <= kMaxDifferenceBytes) { - return "stack pointer is close to top of stack; likely stack overflow."; - } - } - return {}; -} - -static void dump_probable_cause(Tombstone* tombstone, const siginfo_t* si, unwindstack::Maps* maps, - unwindstack::Regs* regs) { - std::optional cause; - if (si->si_signo == SIGSEGV && si->si_code == SEGV_MAPERR) { - if (si->si_addr < reinterpret_cast(4096)) { - cause = "null pointer dereference"; - } else if (si->si_addr == reinterpret_cast(0xffff0ffc)) { - cause = "call to kuser_helper_version"; - } else if (si->si_addr == reinterpret_cast(0xffff0fe0)) { - cause = "call to kuser_get_tls"; - } else if (si->si_addr == reinterpret_cast(0xffff0fc0)) { - cause = "call to kuser_cmpxchg"; - } else if (si->si_addr == reinterpret_cast(0xffff0fa0)) { - cause = "call to kuser_memory_barrier"; - } else if (si->si_addr == reinterpret_cast(0xffff0f60)) { - cause = "call to kuser_cmpxchg64"; - } else { - cause = get_stack_overflow_cause(reinterpret_cast(si->si_addr), regs->sp(), maps); - } - } else if (si->si_signo == SIGSEGV && si->si_code == SEGV_ACCERR) { - uint64_t fault_addr = reinterpret_cast(si->si_addr); - unwindstack::MapInfo* map_info = maps->Find(fault_addr); - if (map_info != nullptr && map_info->flags == PROT_EXEC) { - cause = "execute-only (no-read) memory access error; likely due to data in .text."; - } else { - cause = get_stack_overflow_cause(fault_addr, regs->sp(), maps); - } - } else if (si->si_signo == SIGSYS && si->si_code == SYS_SECCOMP) { - cause = StringPrintf("seccomp prevented call to disallowed %s system call %d", ABI_STRING, - si->si_syscall); - } - - if (cause) { - tombstone->mutable_cause()->set_human_readable(*cause); - } -} - -static void dump_abort_message(Tombstone* tombstone, unwindstack::Unwinder* unwinder, - const ProcessInfo& process_info) { - std::shared_ptr process_memory = unwinder->GetProcessMemory(); - uintptr_t address = process_info.abort_msg_address; - if (address == 0) { - return; - } - - size_t length; - if (!process_memory->ReadFully(address, &length, sizeof(length))) { - PLOG(ERROR) << "Failed to read abort message header"; - return; - } - - // The length field includes the length of the length field itself. - if (length < sizeof(size_t)) { - LOG(ERROR) << "Abort message header malformed: claimed length = " << length; - return; - } - - length -= sizeof(size_t); - - // The abort message should be null terminated already, but reserve a spot for NUL just in case. - std::string msg; - msg.resize(length); - - if (!process_memory->ReadFully(address + sizeof(length), &msg[0], length)) { - PLOG(ERROR) << "Failed to read abort message header"; - return; - } - - tombstone->set_abort_message(msg); -} - -static void dump_open_fds(Tombstone* tombstone, const OpenFilesList* open_files) { - if (open_files) { - for (auto& [fd, entry] : *open_files) { - FD f; - - f.set_fd(fd); - - const std::optional& path = entry.path; - if (path) { - f.set_path(*path); - } - - const std::optional& fdsan_owner = entry.fdsan_owner; - if (fdsan_owner) { - const char* type = android_fdsan_get_tag_type(*fdsan_owner); - uint64_t value = android_fdsan_get_tag_value(*fdsan_owner); - f.set_owner(type); - f.set_tag(value); - } - - *tombstone->add_open_fds() = f; - } - } -} - -static void dump_thread(Tombstone* tombstone, unwindstack::Unwinder* unwinder, - const ThreadInfo& thread_info, bool memory_dump = false) { - Thread thread; - - thread.set_id(thread_info.tid); - thread.set_name(thread_info.thread_name); - - unwindstack::Maps* maps = unwinder->GetMaps(); - unwindstack::Memory* memory = unwinder->GetProcessMemory().get(); - - thread_info.registers->IterateRegisters( - [&thread, memory_dump, maps, memory](const char* name, uint64_t value) { - Register r; - r.set_name(name); - r.set_u64(value); - *thread.add_registers() = r; - - if (memory_dump) { - MemoryDump dump; - - char buf[256]; - size_t start_offset = 0; - ssize_t bytes = dump_memory(buf, sizeof(buf), &start_offset, &value, memory); - if (bytes == -1) { - return; - } - - dump.set_register_name(name); - - unwindstack::MapInfo* map_info = maps->Find(value); - if (map_info) { - dump.set_mapping_name(map_info->name); - } - - dump.set_begin_address(value); - - CHECK(start_offset + bytes <= sizeof(buf)); - dump.set_memory(buf, start_offset + bytes); - - *thread.add_memory_dump() = std::move(dump); - } - }); - - std::unique_ptr regs_copy(thread_info.registers->Clone()); - unwinder->SetRegs(regs_copy.get()); - unwinder->Unwind(); - if (unwinder->NumFrames() == 0) { - LOG(ERROR) << "Failed to unwind"; - if (unwinder->LastErrorCode() != unwindstack::ERROR_NONE) { - LOG(ERROR) << " Error code: " << unwinder->LastErrorCodeString(); - LOG(ERROR) << " Error address: " << StringPrintf("0x%" PRIx64, unwinder->LastErrorAddress()); - } - } else { - unwinder->SetDisplayBuildID(true); - for (const auto& frame : unwinder->frames()) { - BacktraceFrame* f = thread.add_current_backtrace(); - f->set_rel_pc(frame.rel_pc); - f->set_pc(frame.pc); - f->set_sp(frame.sp); - - if (!frame.function_name.empty()) { - // TODO: Should this happen here, or on the display side? - char* demangled_name = - __cxa_demangle(frame.function_name.c_str(), nullptr, nullptr, nullptr); - if (demangled_name) { - f->set_function_name(demangled_name); - free(demangled_name); - } else { - f->set_function_name(frame.function_name); - } - } - - f->set_function_offset(frame.function_offset); - - if (frame.map_start == frame.map_end) { - // No valid map associated with this frame. - f->set_file_name(""); - } else if (!frame.map_name.empty()) { - f->set_file_name(frame.map_name); - } else { - f->set_file_name(StringPrintf("", frame.map_start)); - } - - f->set_file_map_offset(frame.map_elf_start_offset); - - unwindstack::MapInfo* map_info = maps->Find(frame.map_start); - if (map_info) { - f->set_build_id(map_info->GetPrintableBuildID()); - } - } - } - - auto& threads = *tombstone->mutable_threads(); - threads[thread_info.tid] = thread; -} - -static void dump_main_thread(Tombstone* tombstone, unwindstack::Unwinder* unwinder, - const ThreadInfo& thread_info) { - dump_thread(tombstone, unwinder, thread_info, true); -} - -static void dump_mappings(Tombstone* tombstone, unwindstack::Unwinder* unwinder) { - unwindstack::Maps* maps = unwinder->GetMaps(); - std::shared_ptr process_memory = unwinder->GetProcessMemory(); - - for (const auto& map_info : *maps) { - auto* map = tombstone->add_memory_mappings(); - map->set_begin_address(map_info->start); - map->set_end_address(map_info->end); - map->set_offset(map_info->offset); - - if (map_info->flags & PROT_READ) { - map->set_read(true); - } - if (map_info->flags & PROT_WRITE) { - map->set_write(true); - } - if (map_info->flags & PROT_EXEC) { - map->set_execute(true); - } - - map->set_mapping_name(map_info->name); - - std::string build_id = map_info->GetPrintableBuildID(); - if (!build_id.empty()) { - map->set_build_id(build_id); - } - - map->set_load_bias(map_info->GetLoadBias(process_memory)); - } -} - -static void dump_log_file(Tombstone* tombstone, const char* logger, pid_t pid) { - logger_list* logger_list = - android_logger_list_open(android_name_to_log_id(logger), ANDROID_LOG_NONBLOCK, 0, pid); - - LogBuffer buffer; - - while (true) { - log_msg log_entry; - ssize_t actual = android_logger_list_read(logger_list, &log_entry); - - if (actual < 0) { - if (actual == -EINTR) { - // interrupted by signal, retry - continue; - } - if (actual == -EAGAIN) { - // non-blocking EOF; we're done - break; - } else { - ALOGE("Error while reading log: %s\n", strerror(-actual)); - break; - } - } else if (actual == 0) { - ALOGE("Got zero bytes while reading log: %s\n", strerror(errno)); - break; - } - - char timestamp_secs[32]; - time_t sec = static_cast(log_entry.entry.sec); - tm tm; - localtime_r(&sec, &tm); - strftime(timestamp_secs, sizeof(timestamp_secs), "%m-%d %H:%M:%S", &tm); - std::string timestamp = - StringPrintf("%s.%03d", timestamp_secs, log_entry.entry.nsec / 1'000'000); - - // Msg format is: \0\0 - char* msg = log_entry.msg(); - if (msg == nullptr) { - continue; - } - - unsigned char prio = msg[0]; - char* tag = msg + 1; - msg = tag + strlen(tag) + 1; - - // consume any trailing newlines - char* nl = msg + strlen(msg) - 1; - while (nl >= msg && *nl == '\n') { - *nl-- = '\0'; - } - - // Look for line breaks ('\n') and display each text line - // on a separate line, prefixed with the header, like logcat does. - do { - nl = strchr(msg, '\n'); - if (nl != nullptr) { - *nl = '\0'; - ++nl; - } - - LogMessage* log_msg = buffer.add_logs(); - log_msg->set_timestamp(timestamp); - log_msg->set_pid(log_entry.entry.pid); - log_msg->set_tid(log_entry.entry.tid); - log_msg->set_priority(prio); - log_msg->set_tag(tag); - log_msg->set_message(msg); - } while ((msg = nl)); - } - android_logger_list_free(logger_list); - - if (!buffer.logs().empty()) { - buffer.set_name(logger); - *tombstone->add_log_buffers() = std::move(buffer); - } -} - -static void dump_logcat(Tombstone* tombstone, pid_t pid) { - dump_log_file(tombstone, "system", pid); - dump_log_file(tombstone, "main", pid); -} - -void engrave_tombstone_proto(Tombstone* tombstone, unwindstack::Unwinder* unwinder, - const std::map& threads, pid_t target_thread, - const ProcessInfo& process_info, const OpenFilesList* open_files) { - Tombstone result; - - result.set_arch(get_arch()); - result.set_build_fingerprint(android::base::GetProperty("ro.build.fingerprint", "unknown")); - result.set_revision(android::base::GetProperty("ro.revision", "unknown")); - result.set_timestamp(get_timestamp()); - - const ThreadInfo& main_thread = threads.at(target_thread); - result.set_pid(main_thread.pid); - result.set_tid(main_thread.tid); - result.set_uid(main_thread.uid); - result.set_selinux_label(main_thread.selinux_label); - - result.set_process_name(main_thread.process_name); - CHECK(main_thread.siginfo != nullptr); - - Signal sig; - sig.set_number(main_thread.signo); - sig.set_name(get_signame(main_thread.siginfo)); - sig.set_code(main_thread.siginfo->si_code); - sig.set_code_name(get_sigcode(main_thread.siginfo)); - - if (signal_has_sender(main_thread.siginfo, main_thread.pid)) { - sig.set_has_sender(true); - sig.set_sender_uid(main_thread.siginfo->si_uid); - sig.set_sender_pid(main_thread.siginfo->si_pid); - } - - if (process_info.has_fault_address) { - sig.set_has_fault_address(true); - sig.set_fault_address(process_info.fault_address); - } - - *result.mutable_signal_info() = sig; - - dump_abort_message(&result, unwinder, process_info); - - dump_main_thread(&result, unwinder, main_thread); - - for (const auto& [tid, thread_info] : threads) { - if (tid != target_thread) { - dump_thread(&result, unwinder, thread_info); - } - } - - dump_probable_cause(&result, main_thread.siginfo, unwinder->GetMaps(), - main_thread.registers.get()); - - dump_mappings(&result, unwinder); - - // Only dump logs on debuggable devices. - if (android::base::GetBoolProperty("ro.debuggable", false)) { - dump_logcat(&result, main_thread.pid); - } - - dump_open_fds(&result, open_files); - - *tombstone = std::move(result); -} diff --git a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp b/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp deleted file mode 100644 index 5ba0ad6e2..000000000 --- a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp +++ /dev/null @@ -1,361 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include - -#include - -#include -#include -#include -#include -#include -#include - -#include -#include -#include - -#include "tombstone.pb.h" - -using android::base::StringAppendF; -using android::base::StringPrintf; - -#define CB(log, ...) callback(StringPrintf(__VA_ARGS__), log) -#define CBL(...) CB(true, __VA_ARGS__) -#define CBS(...) CB(false, __VA_ARGS__) -using CallbackType = std::function; - -static const char* abi_string(const Tombstone& tombstone) { - switch (tombstone.arch()) { - case Architecture::ARM32: - return "arm"; - case Architecture::ARM64: - return "arm64"; - case Architecture::X86: - return "x86"; - case Architecture::X86_64: - return "x86_64"; - default: - return ""; - } -} - -static int pointer_width(const Tombstone& tombstone) { - switch (tombstone.arch()) { - case Architecture::ARM32: - return 4; - case Architecture::ARM64: - return 8; - case Architecture::X86: - return 4; - case Architecture::X86_64: - return 8; - default: - return 8; - } -} - -static void print_thread_header(CallbackType callback, const Tombstone& tombstone, - const Thread& thread, bool should_log) { - CB(should_log, "pid: %d, tid: %d, name: %s >>> %s <<<", tombstone.pid(), thread.id(), - thread.name().c_str(), tombstone.process_name().c_str()); - CB(should_log, "uid: %d", tombstone.uid()); -} - -static void print_register_row(CallbackType callback, int word_size, - std::vector> row, bool should_log) { - std::string output = " "; - for (const auto& [name, value] : row) { - output += android::base::StringPrintf(" %-3s %0*" PRIx64, name.c_str(), 2 * word_size, - static_cast(value)); - } - callback(output, should_log); -} - -static void print_thread_registers(CallbackType callback, const Tombstone& tombstone, - const Thread& thread, bool should_log) { - static constexpr size_t column_count = 4; - std::vector> current_row; - std::vector> special_row; - std::unordered_set special_registers; - - int word_size = pointer_width(tombstone); - - switch (tombstone.arch()) { - case Architecture::ARM32: - special_registers = {"ip", "lr", "sp", "pc", "pst"}; - break; - - case Architecture::ARM64: - special_registers = {"ip", "lr", "sp", "pc", "pst"}; - break; - - case Architecture::X86: - special_registers = {"ebp", "esp", "eip"}; - break; - - case Architecture::X86_64: - special_registers = {"rbp", "rsp", "rip"}; - break; - - default: - LOG(FATAL) << "unknown architecture"; - } - - for (const auto& reg : thread.registers()) { - auto row = ¤t_row; - if (special_registers.count(reg.name()) == 1) { - row = &special_row; - } - - row->emplace_back(reg.name(), reg.u64()); - if (current_row.size() == column_count) { - print_register_row(callback, word_size, current_row, should_log); - current_row.clear(); - } - } - - if (!current_row.empty()) { - print_register_row(callback, word_size, current_row, should_log); - } - - print_register_row(callback, word_size, special_row, should_log); -} - -static void print_thread_backtrace(CallbackType callback, const Tombstone& tombstone, - const Thread& thread, bool should_log) { - CBS(""); - CB(should_log, "backtrace:"); - int index = 0; - for (const auto& frame : thread.current_backtrace()) { - std::string function; - - if (!frame.function_name().empty()) { - function = - StringPrintf(" (%s+%" PRId64 ")", frame.function_name().c_str(), frame.function_offset()); - } - - std::string build_id; - if (!frame.build_id().empty()) { - build_id = StringPrintf(" (BuildId: %s)", frame.build_id().c_str()); - } - - CB(should_log, " #%02d pc %0*" PRIx64 " %s%s%s", index++, pointer_width(tombstone) * 2, - frame.rel_pc(), frame.file_name().c_str(), function.c_str(), build_id.c_str()); - } -} - -static void print_thread_memory_dump(CallbackType callback, const Tombstone& tombstone, - const Thread& thread) { - static constexpr size_t bytes_per_line = 16; - int word_size = pointer_width(tombstone); - for (const auto& mem : thread.memory_dump()) { - CBS(""); - CBS("memory near %s (%s):", mem.register_name().c_str(), mem.mapping_name().c_str()); - uint64_t addr = mem.begin_address(); - for (size_t offset = 0; offset < mem.memory().size(); offset += bytes_per_line) { - std::string line = StringPrintf(" %0*" PRIx64, word_size * 2, addr + offset); - - size_t bytes = std::min(bytes_per_line, mem.memory().size() - offset); - for (size_t i = 0; i < bytes; i += word_size) { - uint64_t word = 0; - - // Assumes little-endian, but what doesn't? - memcpy(&word, mem.memory().data() + offset + i, word_size); - - StringAppendF(&line, " %0*" PRIx64, word_size * 2, word); - } - - char ascii[bytes_per_line + 1]; - - memset(ascii, '.', sizeof(ascii)); - ascii[bytes_per_line] = '\0'; - - for (size_t i = 0; i < bytes; ++i) { - uint8_t byte = mem.memory()[offset + i]; - if (byte >= 0x20 && byte < 0x7f) { - ascii[i] = byte; - } - } - - CBS("%s %s", line.c_str(), ascii); - } - } -} - -static void print_thread(CallbackType callback, const Tombstone& tombstone, const Thread& thread) { - print_thread_header(callback, tombstone, thread, false); - print_thread_registers(callback, tombstone, thread, false); - print_thread_backtrace(callback, tombstone, thread, false); - print_thread_memory_dump(callback, tombstone, thread); -} - -static void print_main_thread(CallbackType callback, const Tombstone& tombstone, - const Thread& thread) { - print_thread_header(callback, tombstone, thread, true); - - const Signal& signal_info = tombstone.signal_info(); - std::string sender_desc; - - if (signal_info.has_sender()) { - sender_desc = - StringPrintf(" from pid %d, uid %d", signal_info.sender_pid(), signal_info.sender_uid()); - } - - if (!tombstone.has_signal_info()) { - LOG(ERROR) << "signal info missing in tombstone"; - CBL("signal information missing"); - } else { - std::string fault_addr_desc; - if (signal_info.has_fault_address()) { - fault_addr_desc = StringPrintf("0x%" PRIx64, signal_info.fault_address()); - } else { - fault_addr_desc = "--------"; - } - - CBL("signal %d (%s), code %d (%s%s), fault addr %s", signal_info.number(), - signal_info.name().c_str(), signal_info.code(), signal_info.code_name().c_str(), - sender_desc.c_str(), fault_addr_desc.c_str()); - } - - if (tombstone.has_cause()) { - const Cause& cause = tombstone.cause(); - CBL("Cause: %s", cause.human_readable().c_str()); - } - - if (!tombstone.abort_message().empty()) { - CBL("Abort message: '%s'", tombstone.abort_message().c_str()); - } - - print_thread_registers(callback, tombstone, thread, true); - print_thread_backtrace(callback, tombstone, thread, true); - print_thread_memory_dump(callback, tombstone, thread); - - CBS(""); - CBS("memory map (%d %s):", tombstone.memory_mappings().size(), - tombstone.memory_mappings().size() == 1 ? "entry" : "entries"); - int word_size = pointer_width(tombstone); - const auto format_pointer = [word_size](uint64_t ptr) -> std::string { - if (word_size == 8) { - uint64_t top = ptr >> 32; - uint64_t bottom = ptr & 0xFFFFFFFF; - return StringPrintf("%08" PRIx64 "'%08" PRIx64, top, bottom); - } - - return StringPrintf("%0*" PRIx64, word_size * 2, ptr); - }; - - for (const auto& map : tombstone.memory_mappings()) { - std::string line = " "; - StringAppendF(&line, "%s-%s", format_pointer(map.begin_address()).c_str(), - format_pointer(map.end_address() - 1).c_str()); - StringAppendF(&line, " %s%s%s", map.read() ? "r" : "-", map.write() ? "w" : "-", - map.execute() ? "x" : "-"); - StringAppendF(&line, " %8" PRIx64 " %8" PRIx64, map.offset(), - map.end_address() - map.begin_address()); - - if (!map.mapping_name().empty()) { - StringAppendF(&line, " %s", map.mapping_name().c_str()); - - if (!map.build_id().empty()) { - StringAppendF(&line, " (BuildId: %s)", map.build_id().c_str()); - } - - if (map.load_bias() != 0) { - StringAppendF(&line, " (load bias 0x%" PRIx64 ")", map.load_bias()); - } - } - - CBS("%s", line.c_str()); - } -} - -void print_logs(CallbackType callback, const Tombstone& tombstone, int tail) { - for (const auto& buffer : tombstone.log_buffers()) { - if (tail) { - CBS("--------- tail end of log %s", buffer.name().c_str()); - } else { - CBS("--------- log %s", buffer.name().c_str()); - } - - int begin = 0; - if (tail != 0) { - begin = std::max(0, buffer.logs().size() - tail); - } - - for (int i = begin; i < buffer.logs().size(); ++i) { - const LogMessage& msg = buffer.logs(i); - - static const char* kPrioChars = "!.VDIWEFS"; - char priority = (msg.priority() < strlen(kPrioChars) ? kPrioChars[msg.priority()] : '?'); - CBS("%s %5u %5u %c %-8s: %s", msg.timestamp().c_str(), msg.pid(), msg.tid(), priority, - msg.tag().c_str(), msg.message().c_str()); - } - } -} - -bool tombstone_proto_to_text(const Tombstone& tombstone, CallbackType callback) { - CBL("*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***"); - CBL("Build fingerprint: '%s'", tombstone.build_fingerprint().c_str()); - CBL("Revision: '%s'", tombstone.revision().c_str()); - CBL("ABI: '%s'", abi_string(tombstone)); - CBL("Timestamp: %s", tombstone.timestamp().c_str()); - - // Process header - const auto& threads = tombstone.threads(); - auto main_thread_it = threads.find(tombstone.tid()); - if (main_thread_it == threads.end()) { - LOG(ERROR) << "failed to find entry for main thread in tombstone"; - return false; - } - - const auto& main_thread = main_thread_it->second; - - print_main_thread(callback, tombstone, main_thread); - - print_logs(callback, tombstone, 50); - - // protobuf's map is unordered, so sort the keys first. - std::set thread_ids; - for (const auto& [tid, _] : threads) { - if (tid != tombstone.tid()) { - thread_ids.insert(tid); - } - } - - for (const auto& tid : thread_ids) { - CBS("--- --- --- --- --- --- --- --- --- --- --- --- --- --- --- ---"); - print_thread(callback, tombstone, threads.find(tid)->second); - } - - if (tombstone.open_fds().size() > 0) { - CBS(""); - CBS("open files:"); - for (const auto& fd : tombstone.open_fds()) { - std::optional owner; - if (!fd.owner().empty()) { - owner = StringPrintf("owned by %s 0x%" PRIx64, fd.owner().c_str(), fd.tag()); - } - - CBS(" fd %d: %s (%s)", fd.fd(), fd.path().c_str(), owner ? owner->c_str() : "unowned"); - } - } - - print_logs(callback, tombstone, 0); - - return true; -} diff --git a/debuggerd/libdebuggerd/utility.cpp b/debuggerd/libdebuggerd/utility.cpp index f406ad48e..7826efc5e 100644 --- a/debuggerd/libdebuggerd/utility.cpp +++ b/debuggerd/libdebuggerd/utility.cpp @@ -126,56 +126,56 @@ void _VLOG(log_t* log, enum logtype ltype, const char* fmt, va_list ap) { #define MEMORY_BYTES_TO_DUMP 256 #define MEMORY_BYTES_PER_LINE 16 -ssize_t dump_memory(void* out, size_t len, size_t* start_offset, uint64_t* addr, - unwindstack::Memory* memory) { +void dump_memory(log_t* log, unwindstack::Memory* memory, uint64_t addr, const std::string& label) { // Align the address to the number of bytes per line to avoid confusing memory tag output if // memory is tagged and we start from a misaligned address. Start 32 bytes before the address. - *addr &= ~(MEMORY_BYTES_PER_LINE - 1); - if (*addr >= 4128) { - *addr -= 32; + addr &= ~(MEMORY_BYTES_PER_LINE - 1); + if (addr >= 4128) { + addr -= 32; } // We don't want the address tag to appear in the addresses in the memory dump. - *addr = untag_address(*addr); + addr = untag_address(addr); // Don't bother if the address would overflow, taking tag bits into account. Note that // untag_address truncates to 32 bits on 32-bit platforms as a side effect of returning a // uintptr_t, so this also checks for 32-bit overflow. - if (untag_address(*addr + MEMORY_BYTES_TO_DUMP - 1) < *addr) { - return -1; + if (untag_address(addr + MEMORY_BYTES_TO_DUMP - 1) < addr) { + return; } - memset(out, 0, len); - - size_t bytes = memory->Read(*addr, reinterpret_cast(out), len); + // Dump 256 bytes + uintptr_t data[MEMORY_BYTES_TO_DUMP/sizeof(uintptr_t)]; + memset(data, 0, MEMORY_BYTES_TO_DUMP); + size_t bytes = memory->Read(addr, reinterpret_cast(data), sizeof(data)); if (bytes % sizeof(uintptr_t) != 0) { // This should never happen, but just in case. ALOGE("Bytes read %zu, is not a multiple of %zu", bytes, sizeof(uintptr_t)); bytes &= ~(sizeof(uintptr_t) - 1); } - *start_offset = 0; + uint64_t start = 0; bool skip_2nd_read = false; if (bytes == 0) { // In this case, we might want to try another read at the beginning of // the next page only if it's within the amount of memory we would have // read. size_t page_size = sysconf(_SC_PAGE_SIZE); - *start_offset = ((*addr + (page_size - 1)) & ~(page_size - 1)) - *addr; - if (*start_offset == 0 || *start_offset >= len) { + start = ((addr + (page_size - 1)) & ~(page_size - 1)) - addr; + if (start == 0 || start >= MEMORY_BYTES_TO_DUMP) { skip_2nd_read = true; } } - if (bytes < len && !skip_2nd_read) { + if (bytes < MEMORY_BYTES_TO_DUMP && !skip_2nd_read) { // Try to do one more read. This could happen if a read crosses a map, // but the maps do not have any break between them. Or it could happen // if reading from an unreadable map, but the read would cross back // into a readable map. Only requires one extra read because a map has // to contain at least one page, and the total number of bytes to dump // is smaller than a page. - size_t bytes2 = memory->Read(*addr + *start_offset + bytes, static_cast(out) + bytes, - len - bytes - *start_offset); + size_t bytes2 = memory->Read(addr + start + bytes, reinterpret_cast(data) + bytes, + sizeof(data) - bytes - start); bytes += bytes2; if (bytes2 > 0 && bytes % sizeof(uintptr_t) != 0) { // This should never happen, but we'll try and continue any way. @@ -185,21 +185,9 @@ ssize_t dump_memory(void* out, size_t len, size_t* start_offset, uint64_t* addr, } // If we were unable to read anything, it probably means that the register doesn't contain a - // valid pointer. + // valid pointer. In that case, skip the output for this register entirely rather than emitting 16 + // lines of dashes. if (bytes == 0) { - return -1; - } - - return bytes; -} - -void dump_memory(log_t* log, unwindstack::Memory* memory, uint64_t addr, const std::string& label) { - // Dump 256 bytes - uintptr_t data[MEMORY_BYTES_TO_DUMP / sizeof(uintptr_t)]; - size_t start_offset = 0; - - ssize_t bytes = dump_memory(data, sizeof(data), &start_offset, &addr, memory); - if (bytes == -1) { return; } @@ -213,7 +201,7 @@ void dump_memory(log_t* log, unwindstack::Memory* memory, uint64_t addr, const s // words are of course presented differently. uintptr_t* data_ptr = data; size_t current = 0; - size_t total_bytes = start_offset + bytes; + size_t total_bytes = start + bytes; for (size_t line = 0; line < MEMORY_BYTES_TO_DUMP / MEMORY_BYTES_PER_LINE; line++) { uint64_t tagged_addr = addr; long tag = memory->ReadTag(addr); @@ -226,7 +214,7 @@ void dump_memory(log_t* log, unwindstack::Memory* memory, uint64_t addr, const s addr += MEMORY_BYTES_PER_LINE; std::string ascii; for (size_t i = 0; i < MEMORY_BYTES_PER_LINE / sizeof(uintptr_t); i++) { - if (current >= start_offset && current + sizeof(uintptr_t) <= total_bytes) { + if (current >= start && current + sizeof(uintptr_t) <= total_bytes) { android::base::StringAppendF(&logline, " %" PRIPTR, static_cast(*data_ptr)); // Fill out the ascii string from the data. diff --git a/debuggerd/pbtombstone.cpp b/debuggerd/pbtombstone.cpp deleted file mode 100644 index 7527e31e1..000000000 --- a/debuggerd/pbtombstone.cpp +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (C) 2020 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include -#include -#include -#include - -#include -#include - -#include "tombstone.pb.h" - -using android::base::unique_fd; - -[[noreturn]] void usage(bool error) { - fprintf(stderr, "usage: pbtombstone TOMBSTONE.PB\n"); - fprintf(stderr, "Convert a protobuf tombstone to text.\n"); - exit(error); -} - -int main(int argc, const char* argv[]) { - if (argc != 2) { - usage(true); - } - - if (strcmp("-h", argv[1]) == 0 || strcmp("--help", argv[1]) == 0) { - usage(false); - } - - unique_fd fd(open(argv[1], O_RDONLY | O_CLOEXEC)); - if (fd == -1) { - err(1, "failed to open tombstone '%s'", argv[1]); - } - - Tombstone tombstone; - if (!tombstone.ParseFromFileDescriptor(fd.get())) { - err(1, "failed to parse tombstone"); - } - - bool result = tombstone_proto_to_text( - tombstone, [](const std::string& line, bool) { printf("%s\n", line.c_str()); }); - - if (!result) { - errx(1, "tombstone was malformed"); - } - - return 0; -} diff --git a/debuggerd/proto/Android.bp b/debuggerd/proto/Android.bp deleted file mode 100644 index 5307d50d0..000000000 --- a/debuggerd/proto/Android.bp +++ /dev/null @@ -1,28 +0,0 @@ -cc_library { - name: "libtombstone_proto", - cflags: [ - "-Wall", - "-Wextra", - "-Wthread-safety", - "-Werror", - ], - - compile_multilib: "both", - - proto: { - export_proto_headers: true, - type: "lite", - }, - - srcs: [ - "tombstone.proto", - ], - - stl: "libc++_static", - apex_available: [ - "com.android.runtime", - ], - - recovery_available: true, - vendor_ramdisk_available: true, -} diff --git a/debuggerd/proto/tombstone.proto b/debuggerd/proto/tombstone.proto deleted file mode 100644 index aff50bd54..000000000 --- a/debuggerd/proto/tombstone.proto +++ /dev/null @@ -1,118 +0,0 @@ -syntax = "proto3"; - -message Tombstone { - Architecture arch = 1; - string build_fingerprint = 2; - string revision = 3; - string timestamp = 4; - - uint32 pid = 5; - uint32 tid = 6; - uint32 uid = 7; - string selinux_label = 8; - - string process_name = 9; - - Signal signal_info = 10; - string abort_message = 14; - Cause cause = 15; - - map threads = 16; - repeated MemoryMapping memory_mappings = 17; - repeated LogBuffer log_buffers = 18; - repeated FD open_fds = 19; -} - -enum Architecture { - ARM32 = 0; - ARM64 = 1; - X86 = 2; - X86_64 = 3; -} - -message Signal { - int32 number = 1; - string name = 2; - - int32 code = 3; - string code_name = 4; - - bool has_sender = 5; - int32 sender_uid = 6; - int32 sender_pid = 7; - - bool has_fault_address = 8; - uint64 fault_address = 9; -} - -message Cause { - string human_readable = 1; -} - -message Register { - string name = 1; - uint64 u64 = 2; -} - -message Thread { - int32 id = 1; - string name = 2; - repeated Register registers = 3; - repeated BacktraceFrame current_backtrace = 4; - repeated MemoryDump memory_dump = 5; -} - -message BacktraceFrame { - uint64 rel_pc = 1; - uint64 pc = 2; - uint64 sp = 3; - - string function_name = 4; - uint64 function_offset = 5; - - string file_name = 6; - uint64 file_map_offset = 7; - string build_id = 8; -} - -message MemoryDump { - string register_name = 1; - string mapping_name = 2; - uint64 begin_address = 3; - bytes memory = 4; -} - -message MemoryMapping { - uint64 begin_address = 1; - uint64 end_address = 2; - uint64 offset = 3; - - bool read = 4; - bool write = 5; - bool execute = 6; - - string mapping_name = 7; - string build_id = 8; - uint64 load_bias = 9; -} - -message FD { - int32 fd = 1; - string path = 2; - string owner = 3; - uint64 tag = 4; -} - -message LogBuffer { - string name = 1; - repeated LogMessage logs = 2; -} - -message LogMessage { - string timestamp = 1; - uint32 pid = 2; - uint32 tid = 3; - uint32 priority = 4; - string tag = 5; - string message = 6; -} diff --git a/debuggerd/seccomp_policy/crash_dump.arm.policy b/debuggerd/seccomp_policy/crash_dump.arm.policy index 4eac0e92a..254330d51 100644 --- a/debuggerd/seccomp_policy/crash_dump.arm.policy +++ b/debuggerd/seccomp_policy/crash_dump.arm.policy @@ -19,7 +19,6 @@ lseek: 1 getdents64: 1 faccessat: 1 recvmsg: 1 -recvfrom: 1 process_vm_readv: 1 tgkill: 1 rt_sigprocmask: 1 diff --git a/debuggerd/seccomp_policy/crash_dump.arm64.policy b/debuggerd/seccomp_policy/crash_dump.arm64.policy index 1585cc6ee..9b3ef09ef 100644 --- a/debuggerd/seccomp_policy/crash_dump.arm64.policy +++ b/debuggerd/seccomp_policy/crash_dump.arm64.policy @@ -18,7 +18,6 @@ lseek: 1 getdents64: 1 faccessat: 1 recvmsg: 1 -recvfrom: 1 process_vm_readv: 1 tgkill: 1 rt_sigprocmask: 1 diff --git a/debuggerd/seccomp_policy/crash_dump.policy.def b/debuggerd/seccomp_policy/crash_dump.policy.def index cd5aad4cd..2ef31b0b0 100644 --- a/debuggerd/seccomp_policy/crash_dump.policy.def +++ b/debuggerd/seccomp_policy/crash_dump.policy.def @@ -24,7 +24,6 @@ lseek: 1 getdents64: 1 faccessat: 1 recvmsg: 1 -recvfrom: 1 process_vm_readv: 1 diff --git a/debuggerd/seccomp_policy/crash_dump.x86.policy b/debuggerd/seccomp_policy/crash_dump.x86.policy index 4eac0e92a..254330d51 100644 --- a/debuggerd/seccomp_policy/crash_dump.x86.policy +++ b/debuggerd/seccomp_policy/crash_dump.x86.policy @@ -19,7 +19,6 @@ lseek: 1 getdents64: 1 faccessat: 1 recvmsg: 1 -recvfrom: 1 process_vm_readv: 1 tgkill: 1 rt_sigprocmask: 1 diff --git a/debuggerd/seccomp_policy/crash_dump.x86_64.policy b/debuggerd/seccomp_policy/crash_dump.x86_64.policy index 1585cc6ee..9b3ef09ef 100644 --- a/debuggerd/seccomp_policy/crash_dump.x86_64.policy +++ b/debuggerd/seccomp_policy/crash_dump.x86_64.policy @@ -18,7 +18,6 @@ lseek: 1 getdents64: 1 faccessat: 1 recvmsg: 1 -recvfrom: 1 process_vm_readv: 1 tgkill: 1 rt_sigprocmask: 1 From 5ec54d1e843729cd1e38a2f791f001226a653e95 Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:36:12 +0000 Subject: [PATCH 3/5] Revert "tombstoned: support for protobuf fds." Revert "Let crash_dump read /proc/$PID." Revert submission 1556807-tombstone_proto Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Reverted Changes: Ide6811297:tombstoned: switch from goto to RAII. I8d285c4b4:tombstoned: make it easier to add more types of ou... Id0f0fa285:tombstoned: support for protobuf fds. I6be6082ab:Let crash_dump read /proc/$PID. Id812ca390:Make protobuf vendor_ramdisk_available. Ieeece6e6d:libdebuggerd: add protobuf implementation. Change-Id: I0c4f3a17e8b06d6c65255388c571ebf11d371dbb --- debuggerd/common/include/dump_type.h | 6 +- debuggerd/debuggerd_test.cpp | 67 ------------------- .../include/tombstoned/tombstoned.h | 6 +- debuggerd/tombstoned/intercept_manager.cpp | 14 +--- debuggerd/tombstoned/tombstoned.cpp | 63 +++-------------- debuggerd/tombstoned/tombstoned_client.cpp | 23 ++----- 6 files changed, 15 insertions(+), 164 deletions(-) diff --git a/debuggerd/common/include/dump_type.h b/debuggerd/common/include/dump_type.h index a3e171b25..203269e96 100644 --- a/debuggerd/common/include/dump_type.h +++ b/debuggerd/common/include/dump_type.h @@ -24,8 +24,7 @@ enum DebuggerdDumpType : uint8_t { kDebuggerdNativeBacktrace, kDebuggerdTombstone, kDebuggerdJavaBacktrace, - kDebuggerdAnyIntercept, - kDebuggerdTombstoneProto, + kDebuggerdAnyIntercept }; inline std::ostream& operator<<(std::ostream& stream, const DebuggerdDumpType& rhs) { @@ -42,9 +41,6 @@ inline std::ostream& operator<<(std::ostream& stream, const DebuggerdDumpType& r case kDebuggerdAnyIntercept: stream << "kDebuggerdAnyIntercept"; break; - case kDebuggerdTombstoneProto: - stream << "kDebuggerdTombstoneProto"; - break; default: stream << "[unknown]"; } diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index b9d66063e..191931da6 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -1425,70 +1425,3 @@ TEST_F(CrasherTest, stack_overflow) { ConsumeFd(std::move(output_fd), &result); ASSERT_MATCH(result, R"(Cause: stack pointer[^\n]*stack overflow.\n)"); } - -TEST(tombstoned, proto) { - const pid_t self = getpid(); - unique_fd tombstoned_socket, text_fd, proto_fd; - ASSERT_TRUE( - tombstoned_connect(self, &tombstoned_socket, &text_fd, &proto_fd, kDebuggerdTombstoneProto)); - - tombstoned_notify_completion(tombstoned_socket.get()); - - ASSERT_NE(-1, text_fd.get()); - ASSERT_NE(-1, proto_fd.get()); - - struct stat text_st; - ASSERT_EQ(0, fstat(text_fd.get(), &text_st)); - - // Give tombstoned some time to link the files into place. - std::this_thread::sleep_for(100ms); - - // Find the tombstone. - std::optional tombstone_index; - for (int i = 0; i < 50; ++i) { - std::string path = android::base::StringPrintf("/data/tombstones/tombstone_%02d", i); - - struct stat st; - if (TEMP_FAILURE_RETRY(stat(path.c_str(), &st)) != 0) { - continue; - } - - if (st.st_dev == text_st.st_dev && st.st_ino == text_st.st_ino) { - tombstone_index = i; - break; - } - } - - ASSERT_TRUE(tombstone_index); - std::string proto_path = - android::base::StringPrintf("/data/tombstones/tombstone_%02d.pb", *tombstone_index); - - struct stat proto_fd_st; - struct stat proto_file_st; - ASSERT_EQ(0, fstat(proto_fd.get(), &proto_fd_st)); - ASSERT_EQ(0, stat(proto_path.c_str(), &proto_file_st)); - - ASSERT_EQ(proto_fd_st.st_dev, proto_file_st.st_dev); - ASSERT_EQ(proto_fd_st.st_ino, proto_file_st.st_ino); -} - -TEST(tombstoned, proto_intercept) { - const pid_t self = getpid(); - unique_fd intercept_fd, output_fd; - InterceptStatus status; - - tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdTombstone); - ASSERT_EQ(InterceptStatus::kRegistered, status); - - unique_fd tombstoned_socket, text_fd, proto_fd; - ASSERT_TRUE( - tombstoned_connect(self, &tombstoned_socket, &text_fd, &proto_fd, kDebuggerdTombstoneProto)); - ASSERT_TRUE(android::base::WriteStringToFd("foo", text_fd.get())); - tombstoned_notify_completion(tombstoned_socket.get()); - - text_fd.reset(); - - std::string output; - ASSERT_TRUE(android::base::ReadFdToString(output_fd, &output)); - ASSERT_EQ("foo", output); -} diff --git a/debuggerd/tombstoned/include/tombstoned/tombstoned.h b/debuggerd/tombstoned/include/tombstoned/tombstoned.h index bff216c93..6403dbec4 100644 --- a/debuggerd/tombstoned/include/tombstoned/tombstoned.h +++ b/debuggerd/tombstoned/include/tombstoned/tombstoned.h @@ -23,10 +23,6 @@ #include "dump_type.h" bool tombstoned_connect(pid_t pid, android::base::unique_fd* tombstoned_socket, - android::base::unique_fd* text_output_fd, - android::base::unique_fd* proto_output_fd, DebuggerdDumpType dump_type); - -bool tombstoned_connect(pid_t pid, android::base::unique_fd* tombstoned_socket, - android::base::unique_fd* text_output_fd, DebuggerdDumpType dump_type); + android::base::unique_fd* output_fd, DebuggerdDumpType dump_type); bool tombstoned_notify_completion(int tombstoned_socket); diff --git a/debuggerd/tombstoned/intercept_manager.cpp b/debuggerd/tombstoned/intercept_manager.cpp index 437639e3c..6cbf12c83 100644 --- a/debuggerd/tombstoned/intercept_manager.cpp +++ b/debuggerd/tombstoned/intercept_manager.cpp @@ -191,18 +191,6 @@ InterceptManager::InterceptManager(event_base* base, int intercept_socket) : bas /* backlog */ -1, intercept_socket); } -bool dump_types_compatible(DebuggerdDumpType interceptor, DebuggerdDumpType dump) { - if (interceptor == dump) { - return true; - } - - if (interceptor == kDebuggerdTombstone && dump == kDebuggerdTombstoneProto) { - return true; - } - - return false; -} - bool InterceptManager::GetIntercept(pid_t pid, DebuggerdDumpType dump_type, android::base::unique_fd* out_fd) { auto it = this->intercepts.find(pid); @@ -213,7 +201,7 @@ bool InterceptManager::GetIntercept(pid_t pid, DebuggerdDumpType dump_type, if (dump_type == kDebuggerdAnyIntercept) { LOG(INFO) << "found registered intercept of type " << it->second->dump_type << " for requested type kDebuggerdAnyIntercept"; - } else if (!dump_types_compatible(it->second->dump_type, dump_type)) { + } else if (it->second->dump_type != dump_type) { LOG(WARNING) << "found non-matching intercept of type " << it->second->dump_type << " for requested type: " << dump_type; return false; diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index f057260e1..2b74fd489 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/debuggerd/tombstoned/tombstoned.cpp @@ -62,22 +62,14 @@ enum CrashStatus { struct CrashArtifact { unique_fd fd; std::optional temporary_path; - - static CrashArtifact devnull() { - CrashArtifact result; - result.fd.reset(open("/dev/null", O_WRONLY | O_CLOEXEC)); - return result; - } }; struct CrashArtifactPaths { std::string text; - std::optional proto; }; struct CrashOutput { CrashArtifact text; - std::optional proto; }; // Ownership of Crash is a bit messy. @@ -97,15 +89,14 @@ struct Crash { class CrashQueue { public: CrashQueue(const std::string& dir_path, const std::string& file_name_prefix, size_t max_artifacts, - size_t max_concurrent_dumps, bool supports_proto) + size_t max_concurrent_dumps) : file_name_prefix_(file_name_prefix), dir_path_(dir_path), dir_fd_(open(dir_path.c_str(), O_DIRECTORY | O_RDONLY | O_CLOEXEC)), max_artifacts_(max_artifacts), next_artifact_(0), max_concurrent_dumps_(max_concurrent_dumps), - num_concurrent_dumps_(0), - supports_proto_(supports_proto) { + num_concurrent_dumps_(0) { if (dir_fd_ == -1) { PLOG(FATAL) << "failed to open directory: " << dir_path; } @@ -128,14 +119,14 @@ class CrashQueue { static CrashQueue* for_tombstones() { static CrashQueue queue("/data/tombstones", "tombstone_" /* file_name_prefix */, GetIntProperty("tombstoned.max_tombstone_count", 32), - 1 /* max_concurrent_dumps */, true /* supports_proto */); + 1 /* max_concurrent_dumps */); return &queue; } static CrashQueue* for_anrs() { static CrashQueue queue("/data/anr", "trace_" /* file_name_prefix */, GetIntProperty("tombstoned.max_anr_count", 64), - 4 /* max_concurrent_dumps */, false /* supports_proto */); + 4 /* max_concurrent_dumps */); return &queue; } @@ -169,15 +160,6 @@ class CrashQueue { // Don't generate tombstones for backtrace requests. return {}; - case kDebuggerdTombstoneProto: - if (!supports_proto_) { - LOG(ERROR) << "received kDebuggerdTombstoneProto on a queue that doesn't support proto"; - return {}; - } - result.proto = create_temporary_file(); - result.text = create_temporary_file(); - break; - case kDebuggerdTombstone: result.text = create_temporary_file(); break; @@ -196,10 +178,6 @@ class CrashQueue { CrashArtifactPaths result; result.text = StringPrintf("%s%02d", file_name_prefix_.c_str(), next_artifact_); - if (supports_proto_) { - result.proto = StringPrintf("%s%02d.pb", file_name_prefix_.c_str(), next_artifact_); - } - next_artifact_ = (next_artifact_ + 1) % max_artifacts_; return result; } @@ -265,8 +243,6 @@ class CrashQueue { const size_t max_concurrent_dumps_; size_t num_concurrent_dumps_; - bool supports_proto_; - std::deque> queued_requests_; DISALLOW_COPY_AND_ASSIGN(CrashQueue); @@ -285,11 +261,7 @@ static void perform_request(std::unique_ptr crash) { unique_fd output_fd; bool intercepted = intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd); - if (intercepted) { - if (crash->crash_type == kDebuggerdTombstoneProto) { - crash->output.proto = CrashArtifact::devnull(); - } - } else { + if (!intercepted) { if (auto o = CrashQueue::for_crash(crash.get())->get_output(crash->crash_type); o) { crash->output = std::move(*o); output_fd.reset(dup(crash->output.text.fd)); @@ -301,13 +273,8 @@ static void perform_request(std::unique_ptr crash) { TombstonedCrashPacket response = {.packet_type = CrashPacketType::kPerformDump}; - ssize_t rc = -1; - if (crash->output.proto) { - rc = SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get(), - crash->output.proto->fd.get()); - } else { - rc = SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get()); - } + ssize_t rc = + SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get()); output_fd.reset(); @@ -376,7 +343,7 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) { } crash->crash_type = request.packet.dump_request.dump_type; - if (crash->crash_type < 0 || crash->crash_type > kDebuggerdTombstoneProto) { + if (crash->crash_type < 0 || crash->crash_type > kDebuggerdAnyIntercept) { LOG(WARNING) << "unexpected crash dump type: " << crash->crash_type; return; } @@ -471,14 +438,6 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { } } - if (crash->output.proto && crash->output.proto->fd != -1) { - if (!paths.proto) { - LOG(ERROR) << "missing path for proto tombstone"; - } else if (!link_fd(crash->output.proto->fd, queue->dir_fd(), *paths.proto)) { - LOG(ERROR) << "failed to link proto tombstone"; - } - } - // If we don't have O_TMPFILE, we need to clean up after ourselves. if (crash->output.text.temporary_path) { rc = unlinkat(queue->dir_fd().get(), crash->output.text.temporary_path->c_str(), 0); @@ -486,12 +445,6 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { PLOG(ERROR) << "failed to unlink temporary tombstone at " << paths.text; } } - if (crash->output.proto && crash->output.proto->temporary_path) { - rc = unlinkat(queue->dir_fd().get(), crash->output.proto->temporary_path->c_str(), 0); - if (rc != 0) { - PLOG(ERROR) << "failed to unlink temporary proto tombstone"; - } - } } static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) { diff --git a/debuggerd/tombstoned/tombstoned_client.cpp b/debuggerd/tombstoned/tombstoned_client.cpp index abfafb152..2c23c98f3 100644 --- a/debuggerd/tombstoned/tombstoned_client.cpp +++ b/debuggerd/tombstoned/tombstoned_client.cpp @@ -32,13 +32,8 @@ using android::base::ReceiveFileDescriptors; using android::base::unique_fd; -bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* text_output_fd, +bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* output_fd, DebuggerdDumpType dump_type) { - return tombstoned_connect(pid, tombstoned_socket, text_output_fd, nullptr, dump_type); -} - -bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* text_output_fd, - unique_fd* proto_output_fd, DebuggerdDumpType dump_type) { unique_fd sockfd( socket_local_client((dump_type != kDebuggerdJavaBacktrace ? kTombstonedCrashSocketName : kTombstonedJavaTraceSocketName), @@ -59,15 +54,8 @@ bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* text return false; } - unique_fd tmp_output_fd, tmp_proto_fd; - ssize_t rc = -1; - - if (dump_type == kDebuggerdTombstoneProto) { - rc = ReceiveFileDescriptors(sockfd, &packet, sizeof(packet), &tmp_output_fd, &tmp_proto_fd); - } else { - rc = ReceiveFileDescriptors(sockfd, &packet, sizeof(packet), &tmp_output_fd); - } - + unique_fd tmp_output_fd; + ssize_t rc = ReceiveFileDescriptors(sockfd, &packet, sizeof(packet), &tmp_output_fd); if (rc == -1) { async_safe_format_log(ANDROID_LOG_ERROR, "libc", "failed to read response to DumpRequest packet: %s", strerror(errno)); @@ -90,10 +78,7 @@ bool tombstoned_connect(pid_t pid, unique_fd* tombstoned_socket, unique_fd* text } *tombstoned_socket = std::move(sockfd); - *text_output_fd = std::move(tmp_output_fd); - if (proto_output_fd) { - *proto_output_fd = std::move(tmp_proto_fd); - } + *output_fd = std::move(tmp_output_fd); return true; } From eda96eddcbdda9632166232b2363c7b84da0994d Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:36:12 +0000 Subject: [PATCH 4/5] Revert "tombstoned: make it easier to add more types of outputs." Revert "Let crash_dump read /proc/$PID." Revert submission 1556807-tombstone_proto Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Reverted Changes: Ide6811297:tombstoned: switch from goto to RAII. I8d285c4b4:tombstoned: make it easier to add more types of ou... Id0f0fa285:tombstoned: support for protobuf fds. I6be6082ab:Let crash_dump read /proc/$PID. Id812ca390:Make protobuf vendor_ramdisk_available. Ieeece6e6d:libdebuggerd: add protobuf implementation. Change-Id: Ib2403c1b61f6cf0513b76361440fbc5909d7554a --- debuggerd/debuggerd_test.cpp | 4 +- debuggerd/tombstoned/tombstoned.cpp | 151 +++++++++------------------- 2 files changed, 48 insertions(+), 107 deletions(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 191931da6..45e555f44 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -1309,11 +1309,11 @@ TEST(tombstoned, java_trace_intercept_smoke) { tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace); ASSERT_EQ(InterceptStatus::kRegistered, status); - // First connect to tombstoned requesting a native tombstone. This + // First connect to tombstoned requesting a native backtrace. This // should result in a "regular" FD and not the installed intercept. const char native[] = "native"; unique_fd tombstoned_socket, input_fd; - ASSERT_TRUE(tombstoned_connect(self, &tombstoned_socket, &input_fd, kDebuggerdTombstone)); + ASSERT_TRUE(tombstoned_connect(self, &tombstoned_socket, &input_fd, kDebuggerdNativeBacktrace)); ASSERT_TRUE(android::base::WriteFully(input_fd.get(), native, sizeof(native))); tombstoned_notify_completion(tombstoned_socket.get()); diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index 2b74fd489..57e6b6c56 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/debuggerd/tombstoned/tombstoned.cpp @@ -59,26 +59,14 @@ enum CrashStatus { kCrashStatusQueued, }; -struct CrashArtifact { - unique_fd fd; - std::optional temporary_path; -}; - -struct CrashArtifactPaths { - std::string text; -}; - -struct CrashOutput { - CrashArtifact text; -}; - // Ownership of Crash is a bit messy. // It's either owned by an active event that must have a timeout, or owned by // queued_requests, in the case that multiple crashes come in at the same time. struct Crash { ~Crash() { event_free(crash_event); } - CrashOutput output; + std::string crash_tombstone_path; + unique_fd crash_tombstone_fd; unique_fd crash_socket_fd; pid_t crash_pid; event* crash_event = nullptr; @@ -130,56 +118,29 @@ class CrashQueue { return &queue; } - CrashArtifact create_temporary_file() const { - CrashArtifact result; - - std::optional path; - result.fd.reset(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640)); - if (result.fd == -1) { + std::pair get_output() { + std::string path; + unique_fd result(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640)); + if (result == -1) { // We might not have O_TMPFILE. Try creating with an arbitrary filename instead. static size_t counter = 0; std::string tmp_filename = StringPrintf(".temporary%zu", counter++); - result.fd.reset(openat(dir_fd_, tmp_filename.c_str(), - O_WRONLY | O_APPEND | O_CREAT | O_TRUNC | O_CLOEXEC, 0640)); - if (result.fd == -1) { + result.reset(openat(dir_fd_, tmp_filename.c_str(), + O_WRONLY | O_APPEND | O_CREAT | O_TRUNC | O_CLOEXEC, 0640)); + if (result == -1) { PLOG(FATAL) << "failed to create temporary tombstone in " << dir_path_; } - result.temporary_path = std::move(tmp_filename); + path = StringPrintf("%s/%s", dir_path_.c_str(), tmp_filename.c_str()); } - - return std::move(result); + return std::make_pair(std::move(path), std::move(result)); } - std::optional get_output(DebuggerdDumpType dump_type) { - CrashOutput result; - - switch (dump_type) { - case kDebuggerdNativeBacktrace: - case kDebuggerdJavaBacktrace: - // Don't generate tombstones for backtrace requests. - return {}; - - case kDebuggerdTombstone: - result.text = create_temporary_file(); - break; - - default: - LOG(ERROR) << "unexpected dump type: " << dump_type; - return {}; - } - - return result; - } - - borrowed_fd dir_fd() { return dir_fd_; } - - CrashArtifactPaths get_next_artifact_paths() { - CrashArtifactPaths result; - result.text = StringPrintf("%s%02d", file_name_prefix_.c_str(), next_artifact_); - + std::string get_next_artifact_path() { + std::string file_name = + StringPrintf("%s/%s%02d", dir_path_.c_str(), file_name_prefix_.c_str(), next_artifact_); next_artifact_ = (next_artifact_ + 1) % max_artifacts_; - return result; + return file_name; } // Consumes crash if it returns true, otherwise leaves it untouched. @@ -210,8 +171,7 @@ class CrashQueue { time_t oldest_time = std::numeric_limits::max(); for (size_t i = 0; i < max_artifacts_; ++i) { - std::string path = - StringPrintf("%s/%s%02zu", dir_path_.c_str(), file_name_prefix_.c_str(), i); + std::string path = StringPrintf("%s/%s%02zu", dir_path_.c_str(), file_name_prefix_.c_str(), i); struct stat st; if (stat(path.c_str(), &st) != 0) { if (errno == ENOENT) { @@ -252,8 +212,7 @@ class CrashQueue { static constexpr bool kJavaTraceDumpsEnabled = true; // Forward declare the callbacks so they can be placed in a sensible order. -static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int, - void*); +static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int, void*); static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg); static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg); @@ -262,20 +221,20 @@ static void perform_request(std::unique_ptr crash) { bool intercepted = intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd); if (!intercepted) { - if (auto o = CrashQueue::for_crash(crash.get())->get_output(crash->crash_type); o) { - crash->output = std::move(*o); - output_fd.reset(dup(crash->output.text.fd)); + if (crash->crash_type == kDebuggerdNativeBacktrace) { + // Don't generate tombstones for native backtrace requests. + output_fd.reset(open("/dev/null", O_WRONLY | O_CLOEXEC)); } else { - LOG(ERROR) << "failed to get crash output for type " << crash->crash_type; - return; + std::tie(crash->crash_tombstone_path, output_fd) = CrashQueue::for_crash(crash)->get_output(); + crash->crash_tombstone_fd.reset(dup(output_fd.get())); } } - TombstonedCrashPacket response = {.packet_type = CrashPacketType::kPerformDump}; - + TombstonedCrashPacket response = { + .packet_type = CrashPacketType::kPerformDump + }; ssize_t rc = SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get()); - output_fd.reset(); if (rc == -1) { @@ -375,20 +334,8 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) { } } -static bool link_fd(borrowed_fd fd, borrowed_fd dirfd, const std::string& path) { - std::string fd_path = StringPrintf("/proc/self/fd/%d", fd.get()); - - int rc = linkat(AT_FDCWD, fd_path.c_str(), dirfd.get(), path.c_str(), AT_SYMLINK_FOLLOW); - if (rc != 0) { - PLOG(ERROR) << "failed to link file descriptor"; - return false; - } - return true; -} - static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { TombstonedCrashPacket request = {}; - CrashQueue* queue = CrashQueue::for_crash(crash); ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd.get(), &request, sizeof(request))); if (rc == -1) { @@ -406,43 +353,37 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { return; } - if (crash->output.text.fd == -1) { - LOG(WARNING) << "missing output fd"; - return; - } + if (crash->crash_tombstone_fd != -1) { + std::string fd_path = StringPrintf("/proc/self/fd/%d", crash->crash_tombstone_fd.get()); + std::string tombstone_path = CrashQueue::for_crash(crash)->get_next_artifact_path(); - CrashArtifactPaths paths = queue->get_next_artifact_paths(); + // linkat doesn't let us replace a file, so we need to unlink first. + int rc = unlink(tombstone_path.c_str()); + if (rc != 0 && errno != ENOENT) { + PLOG(ERROR) << "failed to unlink tombstone at " << tombstone_path; + return; + } - // Always try to unlink the tombstone file. - // linkat doesn't let us replace a file, so we need to unlink before linking - // our results onto disk, and if we fail for some reason, we should delete - // stale tombstones to avoid confusing inconsistency. - rc = unlinkat(queue->dir_fd().get(), paths.text.c_str(), 0); - if (rc != 0 && errno != ENOENT) { - PLOG(ERROR) << "failed to unlink tombstone at " << paths.text; - return; - } - - if (crash->output.text.fd != -1) { - if (!link_fd(crash->output.text.fd, queue->dir_fd(), paths.text)) { - LOG(ERROR) << "failed to link tombstone"; + rc = linkat(AT_FDCWD, fd_path.c_str(), AT_FDCWD, tombstone_path.c_str(), AT_SYMLINK_FOLLOW); + if (rc != 0) { + PLOG(ERROR) << "failed to link tombstone"; } else { if (crash->crash_type == kDebuggerdJavaBacktrace) { - LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << paths.text; + LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << tombstone_path; } else { // NOTE: Several tools parse this log message to figure out where the // tombstone associated with a given native crash was written. Any changes // to this message must be carefully considered. - LOG(ERROR) << "Tombstone written to: " << paths.text; + LOG(ERROR) << "Tombstone written to: " << tombstone_path; } } - } - // If we don't have O_TMPFILE, we need to clean up after ourselves. - if (crash->output.text.temporary_path) { - rc = unlinkat(queue->dir_fd().get(), crash->output.text.temporary_path->c_str(), 0); - if (rc != 0) { - PLOG(ERROR) << "failed to unlink temporary tombstone at " << paths.text; + // If we don't have O_TMPFILE, we need to clean up after ourselves. + if (!crash->crash_tombstone_path.empty()) { + rc = unlink(crash->crash_tombstone_path.c_str()); + if (rc != 0) { + PLOG(ERROR) << "failed to unlink temporary tombstone at " << crash->crash_tombstone_path; + } } } } @@ -451,7 +392,7 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) { std::unique_ptr crash(static_cast(arg)); CrashQueue* queue = CrashQueue::for_crash(crash); - queue->on_crash_completed(); + CrashQueue::for_crash(crash)->on_crash_completed(); if ((ev & EV_READ) == EV_READ) { crash_completed(sockfd, std::move(crash)); From e156ede145a7fc671c705d045d89b49922a758b5 Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:36:12 +0000 Subject: [PATCH 5/5] Revert "tombstoned: switch from goto to RAII." Revert "Let crash_dump read /proc/$PID." Revert submission 1556807-tombstone_proto Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Reverted Changes: Ide6811297:tombstoned: switch from goto to RAII. I8d285c4b4:tombstoned: make it easier to add more types of ou... Id0f0fa285:tombstoned: support for protobuf fds. I6be6082ab:Let crash_dump read /proc/$PID. Id812ca390:Make protobuf vendor_ramdisk_available. Ieeece6e6d:libdebuggerd: add protobuf implementation. Change-Id: I8a77f6b9e1b42902ef7ee250cc3f1fd341ea0e2b --- debuggerd/tombstoned/tombstoned.cpp | 111 ++++++++++++++-------------- 1 file changed, 56 insertions(+), 55 deletions(-) diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index 57e6b6c56..d09b8e860 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/debuggerd/tombstoned/tombstoned.cpp @@ -48,8 +48,6 @@ using android::base::GetIntProperty; using android::base::SendFileDescriptors; using android::base::StringPrintf; - -using android::base::borrowed_fd; using android::base::unique_fd; static InterceptManager* intercept_manager; @@ -100,10 +98,6 @@ class CrashQueue { return (crash->crash_type == kDebuggerdJavaBacktrace) ? for_anrs() : for_tombstones(); } - static CrashQueue* for_crash(const std::unique_ptr& crash) { - return for_crash(crash.get()); - } - static CrashQueue* for_tombstones() { static CrashQueue queue("/data/tombstones", "tombstone_" /* file_name_prefix */, GetIntProperty("tombstoned.max_tombstone_count", 32), @@ -143,21 +137,20 @@ class CrashQueue { return file_name; } - // Consumes crash if it returns true, otherwise leaves it untouched. - bool maybe_enqueue_crash(std::unique_ptr&& crash) { + bool maybe_enqueue_crash(Crash* crash) { if (num_concurrent_dumps_ == max_concurrent_dumps_) { - queued_requests_.emplace_back(std::move(crash)); + queued_requests_.push_back(crash); return true; } return false; } - void maybe_dequeue_crashes(void (*handler)(std::unique_ptr crash)) { + void maybe_dequeue_crashes(void (*handler)(Crash* crash)) { while (!queued_requests_.empty() && num_concurrent_dumps_ < max_concurrent_dumps_) { - std::unique_ptr next_crash = std::move(queued_requests_.front()); + Crash* next_crash = queued_requests_.front(); queued_requests_.pop_front(); - handler(std::move(next_crash)); + handler(next_crash); } } @@ -203,7 +196,7 @@ class CrashQueue { const size_t max_concurrent_dumps_; size_t num_concurrent_dumps_; - std::deque> queued_requests_; + std::deque queued_requests_; DISALLOW_COPY_AND_ASSIGN(CrashQueue); }; @@ -216,7 +209,7 @@ static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, so static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg); static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg); -static void perform_request(std::unique_ptr crash) { +static void perform_request(Crash* crash) { unique_fd output_fd; bool intercepted = intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd); @@ -239,24 +232,25 @@ static void perform_request(std::unique_ptr crash) { if (rc == -1) { PLOG(WARNING) << "failed to send response to CrashRequest"; - return; + goto fail; } else if (rc != sizeof(response)) { PLOG(WARNING) << "crash socket write returned short"; - return; + goto fail; + } else { + // TODO: Make this configurable by the interceptor? + struct timeval timeout = { 10, 0 }; + + event_base* base = event_get_base(crash->crash_event); + event_assign(crash->crash_event, base, crash->crash_socket_fd, EV_TIMEOUT | EV_READ, + crash_completed_cb, crash); + event_add(crash->crash_event, &timeout); } - // TODO: Make this configurable by the interceptor? - struct timeval timeout = {10, 0}; - - event_base* base = event_get_base(crash->crash_event); - - event_assign(crash->crash_event, base, crash->crash_socket_fd, EV_TIMEOUT | EV_READ, - crash_completed_cb, crash.get()); - event_add(crash->crash_event, &timeout); CrashQueue::for_crash(crash)->on_crash_started(); + return; - // The crash is now owned by the event loop. - crash.release(); +fail: + delete crash; } static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int, @@ -274,37 +268,39 @@ static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, so } static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) { - std::unique_ptr crash(static_cast(arg)); + ssize_t rc; + Crash* crash = static_cast(arg); + TombstonedCrashPacket request = {}; if ((ev & EV_TIMEOUT) != 0) { LOG(WARNING) << "crash request timed out"; - return; + goto fail; } else if ((ev & EV_READ) == 0) { LOG(WARNING) << "tombstoned received unexpected event from crash socket"; - return; + goto fail; } - ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request))); + rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request))); if (rc == -1) { PLOG(WARNING) << "failed to read from crash socket"; - return; + goto fail; } else if (rc != sizeof(request)) { LOG(WARNING) << "crash socket received short read of length " << rc << " (expected " << sizeof(request) << ")"; - return; + goto fail; } if (request.packet_type != CrashPacketType::kDumpRequest) { LOG(WARNING) << "unexpected crash packet type, expected kDumpRequest, received " << StringPrintf("%#2hhX", request.packet_type); - return; + goto fail; } crash->crash_type = request.packet.dump_request.dump_type; if (crash->crash_type < 0 || crash->crash_type > kDebuggerdAnyIntercept) { LOG(WARNING) << "unexpected crash dump type: " << crash->crash_type; - return; + goto fail; } if (crash->crash_type != kDebuggerdJavaBacktrace) { @@ -318,39 +314,51 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) { int ret = getsockopt(sockfd, SOL_SOCKET, SO_PEERCRED, &cr, &len); if (ret != 0) { PLOG(ERROR) << "Failed to getsockopt(..SO_PEERCRED)"; - return; + goto fail; } crash->crash_pid = cr.pid; } - pid_t crash_pid = crash->crash_pid; - LOG(INFO) << "received crash request for pid " << crash_pid; + LOG(INFO) << "received crash request for pid " << crash->crash_pid; - if (CrashQueue::for_crash(crash)->maybe_enqueue_crash(std::move(crash))) { - LOG(INFO) << "enqueueing crash request for pid " << crash_pid; + if (CrashQueue::for_crash(crash)->maybe_enqueue_crash(crash)) { + LOG(INFO) << "enqueueing crash request for pid " << crash->crash_pid; } else { - perform_request(std::move(crash)); + perform_request(crash); } + + return; + +fail: + delete crash; } -static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { +static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) { + ssize_t rc; + Crash* crash = static_cast(arg); TombstonedCrashPacket request = {}; - ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd.get(), &request, sizeof(request))); + CrashQueue::for_crash(crash)->on_crash_completed(); + + if ((ev & EV_READ) == 0) { + goto fail; + } + + rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request))); if (rc == -1) { PLOG(WARNING) << "failed to read from crash socket"; - return; + goto fail; } else if (rc != sizeof(request)) { LOG(WARNING) << "crash socket received short read of length " << rc << " (expected " << sizeof(request) << ")"; - return; + goto fail; } if (request.packet_type != CrashPacketType::kCompletedDump) { LOG(WARNING) << "unexpected crash packet type, expected kCompletedDump, received " << uint32_t(request.packet_type); - return; + goto fail; } if (crash->crash_tombstone_fd != -1) { @@ -361,7 +369,7 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { int rc = unlink(tombstone_path.c_str()); if (rc != 0 && errno != ENOENT) { PLOG(ERROR) << "failed to unlink tombstone at " << tombstone_path; - return; + goto fail; } rc = linkat(AT_FDCWD, fd_path.c_str(), AT_FDCWD, tombstone_path.c_str(), AT_SYMLINK_FOLLOW); @@ -386,17 +394,10 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { } } } -} -static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) { - std::unique_ptr crash(static_cast(arg)); +fail: CrashQueue* queue = CrashQueue::for_crash(crash); - - CrashQueue::for_crash(crash)->on_crash_completed(); - - if ((ev & EV_READ) == EV_READ) { - crash_completed(sockfd, std::move(crash)); - } + delete crash; // If there's something queued up, let them proceed. queue->maybe_dequeue_crashes(perform_request);