From 6f9eeecd2b7d0e194bd710a8bdc0222ebe35d28d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 12 Sep 2018 13:55:47 -0700 Subject: [PATCH] Fix multithreaded backtraces for seccomp processes. Add threads to the existing seccomp backtrace test to prevent regressing this. Bug: http://b/114139908 Bug: http://b/115349586 Test: debuggerd_test32 Test: debuggerd_test64 Change-Id: I07fbe1619b60f0008deb045a249f9045404478c2 --- debuggerd/debuggerd_test.cpp | 44 +++++++++++++++++++++--- debuggerd/handler/debuggerd_fallback.cpp | 7 ++-- debuggerd/handler/debuggerd_handler.cpp | 13 ++++--- debuggerd/handler/fallback.h | 22 ++++++++++++ 4 files changed, 75 insertions(+), 11 deletions(-) create mode 100644 debuggerd/handler/fallback.h diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index e2ea480e5..388facb45 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -587,9 +587,28 @@ static const char* const kDebuggerdSeccompPolicy = "/system/etc/seccomp_policy/crash_dump." ABI_STRING ".policy"; static pid_t seccomp_fork_impl(void (*prejail)()) { - unique_fd policy_fd(open(kDebuggerdSeccompPolicy, O_RDONLY | O_CLOEXEC)); - if (policy_fd == -1) { - LOG(FATAL) << "failed to open policy " << kDebuggerdSeccompPolicy; + std::string policy; + if (!android::base::ReadFileToString(kDebuggerdSeccompPolicy, &policy)) { + PLOG(FATAL) << "failed to read policy file"; + } + + // Allow a bunch of syscalls used by the tests. + policy += "\nclone: 1"; + policy += "\nsigaltstack: 1"; + policy += "\nnanosleep: 1"; + + FILE* tmp_file = tmpfile(); + if (!tmp_file) { + PLOG(FATAL) << "tmpfile failed"; + } + + unique_fd tmp_fd(dup(fileno(tmp_file))); + if (!android::base::WriteStringToFd(policy, tmp_fd.get())) { + PLOG(FATAL) << "failed to write policy to tmpfile"; + } + + if (lseek(tmp_fd.get(), 0, SEEK_SET) != 0) { + PLOG(FATAL) << "failed to seek tmp_fd"; } ScopedMinijail jail{minijail_new()}; @@ -600,7 +619,7 @@ static pid_t seccomp_fork_impl(void (*prejail)()) { minijail_no_new_privs(jail.get()); minijail_log_seccomp_filter_failures(jail.get()); minijail_use_seccomp_filter(jail.get()); - minijail_parse_seccomp_filters_from_fd(jail.get(), policy_fd.release()); + minijail_parse_seccomp_filters_from_fd(jail.get(), tmp_fd.release()); pid_t result = fork(); if (result == -1) { @@ -735,6 +754,16 @@ TEST_F(CrasherTest, seccomp_tombstone) { ASSERT_BACKTRACE_FRAME(result, "raise_debugger_signal"); } +extern "C" void foo() { + LOG(INFO) << "foo"; + std::this_thread::sleep_for(1s); +} + +extern "C" void bar() { + LOG(INFO) << "bar"; + std::this_thread::sleep_for(1s); +} + TEST_F(CrasherTest, seccomp_backtrace) { int intercept_result; unique_fd output_fd; @@ -742,6 +771,11 @@ TEST_F(CrasherTest, seccomp_backtrace) { static const auto dump_type = kDebuggerdNativeBacktrace; StartProcess( []() { + std::thread a(foo); + std::thread b(bar); + + std::this_thread::sleep_for(100ms); + raise_debugger_signal(dump_type); _exit(0); }, @@ -756,6 +790,8 @@ TEST_F(CrasherTest, seccomp_backtrace) { 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"); } TEST_F(CrasherTest, seccomp_crash_logcat) { diff --git a/debuggerd/handler/debuggerd_fallback.cpp b/debuggerd/handler/debuggerd_fallback.cpp index 079a574d3..ed7423b37 100644 --- a/debuggerd/handler/debuggerd_fallback.cpp +++ b/debuggerd/handler/debuggerd_fallback.cpp @@ -47,6 +47,7 @@ #include #include "debuggerd/handler.h" +#include "handler/fallback.h" #include "tombstoned/tombstoned.h" #include "util.h" @@ -187,7 +188,7 @@ static std::pair unpack_thread_fd(uint64_t value) { static void trace_handler(siginfo_t* info, ucontext_t* ucontext) { static std::atomic trace_output(pack_thread_fd(-1, -1)); - if (info->si_value.sival_int == ~0) { + if (info->si_value.sival_ptr == kDebuggerdFallbackSivalPtrRequestDump) { // Asked to dump by the original signal recipient. uint64_t val = trace_output.load(); auto [tid, fd] = unpack_thread_fd(val); @@ -259,7 +260,7 @@ static void trace_handler(siginfo_t* info, ucontext_t* ucontext) { siginfo_t siginfo = {}; siginfo.si_code = SI_QUEUE; - siginfo.si_value.sival_int = ~0; + siginfo.si_value.sival_ptr = kDebuggerdFallbackSivalPtrRequestDump; siginfo.si_pid = getpid(); siginfo.si_uid = getuid(); @@ -331,7 +332,7 @@ static void crash_handler(siginfo_t* info, ucontext_t* ucontext, void* abort_mes extern "C" void debuggerd_fallback_handler(siginfo_t* info, ucontext_t* ucontext, void* abort_message) { - if (info->si_signo == DEBUGGER_SIGNAL && info->si_value.sival_int != 0) { + if (info->si_signo == DEBUGGER_SIGNAL && info->si_value.sival_ptr != nullptr) { return trace_handler(info, ucontext); } else { return crash_handler(info, ucontext, abort_message); diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index 15557b6d8..a064ca0f7 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -58,6 +58,8 @@ #include "dump_type.h" #include "protocol.h" +#include "handler/fallback.h" + using android::base::Pipe; // We muck with our fds in a 'thread' that doesn't share the same fd table. @@ -473,13 +475,15 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c } void* abort_message = nullptr; + uintptr_t si_val = reinterpret_cast(info->si_ptr); if (signal_number == DEBUGGER_SIGNAL) { if (info->si_code == SI_QUEUE && info->si_pid == __getpid()) { // Allow for the abort message to be explicitly specified via the sigqueue value. // Keep the bottom bit intact for representing whether we want a backtrace or a tombstone. - uintptr_t value = reinterpret_cast(info->si_ptr); - abort_message = reinterpret_cast(value & ~1); - info->si_ptr = reinterpret_cast(value & 1); + if (si_val != kDebuggerdFallbackSivalUintptrRequestDump) { + abort_message = reinterpret_cast(si_val & ~1); + info->si_ptr = reinterpret_cast(si_val & 1); + } } } else { if (g_callbacks.get_abort_message) { @@ -492,7 +496,8 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c // of a specific thread. It is possible that the prctl call might return 1, // then return 0 in subsequent calls, so check the sival_int to determine if // the fallback handler should be called first. - if (info->si_value.sival_int == ~0 || prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0) == 1) { + if (si_val == kDebuggerdFallbackSivalUintptrRequestDump || + prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0) == 1) { // This check might be racy if another thread sets NO_NEW_PRIVS, but this should be unlikely, // you can only set NO_NEW_PRIVS to 1, and the effect should be at worst a single missing // ANR trace. diff --git a/debuggerd/handler/fallback.h b/debuggerd/handler/fallback.h new file mode 100644 index 000000000..597f5824c --- /dev/null +++ b/debuggerd/handler/fallback.h @@ -0,0 +1,22 @@ +/* + * Copyright 2018 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. + */ + +#pragma once + +#include + +static void* const kDebuggerdFallbackSivalPtrRequestDump = reinterpret_cast(~0UL); +static const uintptr_t kDebuggerdFallbackSivalUintptrRequestDump = ~0UL;