From 92abfb41f3c96272268c065029275b39297d82a7 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 4 May 2017 17:12:57 -0700 Subject: [PATCH] debuggerd_handler: use syscall(__NR_get[pt]id) instead of get[pt]id. bionic's cached values for getpid/gettid can be invalid if the crashing process manually invoked clone to create a thread or process, which will lead the crash_dump refusing to do anything, because it sees the actual values. Use the getpid/gettid syscalls directly to ensure correct values on this end. Bug: http://b/37769298 Test: debuggerd_test Change-Id: I0b1e652beb1a66e564a48b88ed7fa971d61c6ff9 (cherry picked from commit 2e7b8e2d1aff139895127a93c020bddb98a0f26e) --- debuggerd/debuggerd_test.cpp | 38 +++++++++++++++++++++++-- debuggerd/handler/debuggerd_handler.cpp | 20 +++++++++---- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 568879ed5..0b4bbfb60 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -29,6 +29,7 @@ #include #include +#include #include #include #include @@ -149,7 +150,7 @@ class CrasherTest : public ::testing::Test { // Returns -1 if we fail to read a response from tombstoned, otherwise the received return code. void FinishIntercept(int* result); - void StartProcess(std::function function); + void StartProcess(std::function function, std::function forker = fork); void StartCrasher(const std::string& crash_type); void FinishCrasher(); void AssertDeath(int signo); @@ -195,14 +196,14 @@ void CrasherTest::FinishIntercept(int* result) { } } -void CrasherTest::StartProcess(std::function function) { +void CrasherTest::StartProcess(std::function function, std::function forker) { unique_fd read_pipe; unique_fd crasher_read_pipe; if (!Pipe(&crasher_read_pipe, &crasher_pipe)) { FAIL() << "failed to create pipe: " << strerror(errno); } - crasher_pid = fork(); + crasher_pid = forker(); if (crasher_pid == -1) { FAIL() << "fork failed: " << strerror(errno); } else if (crasher_pid == 0) { @@ -527,6 +528,37 @@ TEST_F(CrasherTest, capabilities) { ASSERT_MATCH(result, R"(#00 pc [0-9a-f]+\s+ /system/lib)" ARCH_SUFFIX R"(/libc.so \(tgkill)"); } +TEST_F(CrasherTest, fake_pid) { + int intercept_result; + unique_fd output_fd; + + // Prime the getpid/gettid caches. + UNUSED(getpid()); + UNUSED(gettid()); + + std::function clone_fn = []() { + return syscall(__NR_clone, SIGCHLD, nullptr, nullptr, nullptr, nullptr); + }; + StartProcess( + []() { + ASSERT_NE(getpid(), syscall(__NR_getpid)); + ASSERT_NE(gettid(), syscall(__NR_gettid)); + raise(SIGSEGV); + }, + clone_fn); + + StartIntercept(&output_fd); + FinishCrasher(); + AssertDeath(SIGSEGV); + FinishIntercept(&intercept_result); + + ASSERT_EQ(1, intercept_result) << "tombstoned reported failure"; + + std::string result; + ConsumeFd(std::move(output_fd), &result); + ASSERT_MATCH(result, R"(#00 pc [0-9a-f]+\s+ /system/lib)" ARCH_SUFFIX R"(/libc.so \(tgkill)"); +} + TEST(crash_dump, zombie) { pid_t forkpid = fork(); diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index b70554f66..d58c73d65 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -62,6 +62,16 @@ #define CRASH_DUMP_PATH "/system/bin/" CRASH_DUMP_NAME +// Wrappers that directly invoke the respective syscalls, in case the cached values are invalid. +#pragma GCC poison getpid gettid +static pid_t __getpid() { + return syscall(__NR_getpid); +} + +static pid_t __gettid() { + return syscall(__NR_gettid); +} + class ErrnoRestorer { public: ErrnoRestorer() : saved_errno_(errno) { @@ -120,7 +130,7 @@ static void log_signal_summary(int signum, const siginfo_t* info) { } if (signum == DEBUGGER_SIGNAL) { - __libc_format_log(ANDROID_LOG_INFO, "libc", "Requested dump for tid %d (%s)", gettid(), + __libc_format_log(ANDROID_LOG_INFO, "libc", "Requested dump for tid %d (%s)", __gettid(), thread_name); return; } @@ -173,7 +183,7 @@ static void log_signal_summary(int signum, const siginfo_t* info) { } __libc_format_log(ANDROID_LOG_FATAL, "libc", "Fatal signal %d (%s)%s%s in tid %d (%s)", signum, - signal_name, code_desc, addr_desc, gettid(), thread_name); + signal_name, code_desc, addr_desc, __gettid(), thread_name); } /* @@ -331,7 +341,7 @@ static void resend_signal(siginfo_t* info, bool crash_dump_started) { // rt_tgsigqueueinfo(2) to preserve SA_SIGINFO) will cause it to be delivered // when our signal handler returns. if (crash_dump_started || info->si_signo != DEBUGGER_SIGNAL) { - int rc = syscall(SYS_rt_tgsigqueueinfo, getpid(), gettid(), info->si_signo, info); + int rc = syscall(SYS_rt_tgsigqueueinfo, __getpid(), __gettid(), info->si_signo, info); if (rc != 0) { fatal_errno("failed to resend signal during crash"); } @@ -356,7 +366,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c memset(&si, 0, sizeof(si)); si.si_signo = signal_number; si.si_code = SI_USER; - si.si_pid = getpid(); + si.si_pid = __getpid(); si.si_uid = getuid(); info = &si; } else if (info->si_code >= 0 || info->si_code == SI_TKILL) { @@ -398,7 +408,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void* c debugger_thread_info thread_info = { .crash_dump_started = false, .pseudothread_tid = -1, - .crashing_tid = gettid(), + .crashing_tid = __gettid(), .signal_number = signal_number, .info = info };