From 7c2e7e31f6a544a6051e4d3f4e26a22a1c0707b7 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 27 May 2022 12:57:58 -0700 Subject: [PATCH] Fix fallback paths for dumping threads. In the fallback path, if the non-main thread is the target to be dumped, then no other threads are dumped when creating a tombstone. Fix this and add unit tests to verify that this all threads, including the main thread are dumped. Bug: 234058038 Test: All unit tests pass. Test: debuggerd -b media.swcodec process Test: debuggerd media.swcodec process Change-Id: Ibb75264f7b3847acdbab939a66902d986c0d0e5c --- debuggerd/debuggerd_test.cpp | 65 ++++++++++++++++++++++++ debuggerd/handler/debuggerd_fallback.cpp | 5 +- debuggerd/libdebuggerd/tombstone.cpp | 54 ++++++++++---------- debuggerd/util.cpp | 4 +- 4 files changed, 98 insertions(+), 30 deletions(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index e11330819..ed90ab468 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -1486,6 +1486,37 @@ TEST_F(CrasherTest, seccomp_tombstone_thread_abort) { ASSERT_BACKTRACE_FRAME(result, "abort"); } +TEST_F(CrasherTest, seccomp_tombstone_multiple_threads_abort) { + int intercept_result; + unique_fd output_fd; + + static const auto dump_type = kDebuggerdTombstone; + StartProcess( + []() { + std::thread a(foo); + std::thread b(bar); + + std::this_thread::sleep_for(100ms); + + std::thread abort_thread([] { abort(); }); + abort_thread.join(); + }, + &seccomp_fork); + + StartIntercept(&output_fd, dump_type); + FinishCrasher(); + AssertDeath(SIGABRT); + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_BACKTRACE_FRAME(result, "abort"); + ASSERT_BACKTRACE_FRAME(result, "foo"); + ASSERT_BACKTRACE_FRAME(result, "bar"); + ASSERT_BACKTRACE_FRAME(result, "main"); +} + TEST_F(CrasherTest, seccomp_backtrace) { int intercept_result; unique_fd output_fd; @@ -1516,6 +1547,40 @@ TEST_F(CrasherTest, seccomp_backtrace) { ASSERT_BACKTRACE_FRAME(result, "bar"); } +TEST_F(CrasherTest, seccomp_backtrace_from_thread) { + int intercept_result; + unique_fd output_fd; + + static const auto dump_type = kDebuggerdNativeBacktrace; + StartProcess( + []() { + std::thread a(foo); + std::thread b(bar); + + std::this_thread::sleep_for(100ms); + + std::thread raise_thread([] { + raise_debugger_signal(dump_type); + _exit(0); + }); + raise_thread.join(); + }, + &seccomp_fork); + + StartIntercept(&output_fd, dump_type); + FinishCrasher(); + AssertDeath(0); + FinishIntercept(&intercept_result); + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_BACKTRACE_FRAME(result, "raise_debugger_signal"); + ASSERT_BACKTRACE_FRAME(result, "foo"); + ASSERT_BACKTRACE_FRAME(result, "bar"); + ASSERT_BACKTRACE_FRAME(result, "main"); +} + TEST_F(CrasherTest, seccomp_crash_logcat) { StartProcess([]() { abort(); }, &seccomp_fork); FinishCrasher(); diff --git a/debuggerd/handler/debuggerd_fallback.cpp b/debuggerd/handler/debuggerd_fallback.cpp index 70e3022fe..4721391ef 100644 --- a/debuggerd/handler/debuggerd_fallback.cpp +++ b/debuggerd/handler/debuggerd_fallback.cpp @@ -210,7 +210,10 @@ static void trace_handler(siginfo_t* info, ucontext_t* ucontext) { // Send a signal to all of our siblings, asking them to dump their stack. pid_t current_tid = gettid(); - if (!iterate_tids(current_tid, [&output_fd](pid_t tid) { + if (!iterate_tids(current_tid, [&output_fd, ¤t_tid](pid_t tid) { + if (current_tid == tid) { + return; + } // Use a pipe, to be able to detect situations where the thread gracefully exits before // receiving our signal. unique_fd pipe_read, pipe_write; diff --git a/debuggerd/libdebuggerd/tombstone.cpp b/debuggerd/libdebuggerd/tombstone.cpp index 5ca2c00e8..e5b4d74a1 100644 --- a/debuggerd/libdebuggerd/tombstone.cpp +++ b/debuggerd/libdebuggerd/tombstone.cpp @@ -55,15 +55,15 @@ void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_m siginfo_t* siginfo, ucontext_t* ucontext) { pid_t uid = getuid(); pid_t pid = getpid(); - pid_t tid = gettid(); + pid_t target_tid = gettid(); log_t log; - log.current_tid = tid; - log.crashed_tid = tid; + log.current_tid = target_tid; + log.crashed_tid = target_tid; log.tfd = tombstone_fd; log.amfd_data = nullptr; - std::string thread_name = get_thread_name(tid); + std::string thread_name = get_thread_name(target_tid); std::vector command_line = get_command_line(pid); std::unique_ptr regs( @@ -73,32 +73,34 @@ void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_m 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), - .pid = pid, .command_line = std::move(command_line), .selinux_label = std::move(selinux_label), - .siginfo = siginfo, + threads[target_tid] = ThreadInfo { + .registers = std::move(regs), .uid = uid, .tid = target_tid, + .thread_name = std::move(thread_name), .pid = pid, .command_line = std::move(command_line), + .selinux_label = std::move(selinux_label), .siginfo = siginfo, #if defined(__aarch64__) // Only supported on aarch64 for now. .tagged_addr_ctrl = prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0), .pac_enabled_keys = prctl(PR_PAC_GET_ENABLED_KEYS, 0, 0, 0, 0), #endif }; - if (pid == tid) { - const ThreadInfo& thread = threads[pid]; - if (!iterate_tids(pid, [&threads, &thread](pid_t tid) { - threads[tid] = ThreadInfo{ - .uid = thread.uid, - .tid = tid, - .pid = thread.pid, - .command_line = thread.command_line, - .thread_name = get_thread_name(tid), - .tagged_addr_ctrl = thread.tagged_addr_ctrl, - .pac_enabled_keys = thread.pac_enabled_keys, - }; - })) { - async_safe_format_log(ANDROID_LOG_ERROR, LOG_TAG, "failed to open /proc/%d/task: %s", pid, - strerror(errno)); - } + const ThreadInfo& thread = threads[pid]; + if (!iterate_tids(pid, [&threads, &thread, &target_tid](pid_t tid) { + if (target_tid == tid) { + return; + } + async_safe_format_log(ANDROID_LOG_ERROR, LOG_TAG, "Adding thread %d", tid); + threads[tid] = ThreadInfo{ + .uid = thread.uid, + .tid = tid, + .pid = thread.pid, + .command_line = thread.command_line, + .thread_name = get_thread_name(tid), + .tagged_addr_ctrl = thread.tagged_addr_ctrl, + .pac_enabled_keys = thread.pac_enabled_keys, + }; + })) { + async_safe_format_log(ANDROID_LOG_ERROR, LOG_TAG, "failed to open /proc/%d/task: %s", pid, + strerror(errno)); } // Do not use the thread cache here because it will call pthread_key_create @@ -116,8 +118,8 @@ void engrave_tombstone_ucontext(int tombstone_fd, int proto_fd, uint64_t abort_m ProcessInfo process_info; 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)), unique_fd(dup(proto_fd)), &unwinder, threads, + target_tid, process_info, nullptr, nullptr); } void engrave_tombstone(unique_fd output_fd, unique_fd proto_fd, diff --git a/debuggerd/util.cpp b/debuggerd/util.cpp index 5c6abc94b..df033dfcc 100644 --- a/debuggerd/util.cpp +++ b/debuggerd/util.cpp @@ -90,9 +90,7 @@ bool iterate_tids(pid_t pid, std::function callback) { if (tid == 0) { continue; } - if (pid != tid) { - callback(tid); - } + callback(tid); } return true; }