diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 3b2419365..e7503e97e 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -17,6 +17,7 @@ #include #include #include +#include #include #include @@ -90,6 +91,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 StartCrasher(const std::string& crash_type); void FinishCrasher(); void AssertDeath(int signo); @@ -166,9 +168,8 @@ void CrasherTest::FinishIntercept(int* result) { } } -void CrasherTest::StartCrasher(const std::string& crash_type) { - std::string type = "wait-" + crash_type; - +void CrasherTest::StartProcess(std::function function) { + unique_fd read_pipe; unique_fd crasher_read_pipe; if (!Pipe(&crasher_read_pipe, &crasher_pipe)) { FAIL() << "failed to create pipe: " << strerror(errno); @@ -182,9 +183,17 @@ void CrasherTest::StartCrasher(const std::string& crash_type) { dup2(crasher_read_pipe.get(), STDIN_FILENO); dup2(devnull.get(), STDOUT_FILENO); dup2(devnull.get(), STDERR_FILENO); + function(); + _exit(0); + } +} + +void CrasherTest::StartCrasher(const std::string& crash_type) { + std::string type = "wait-" + crash_type; + StartProcess([type]() { execl(CRASHER_PATH, CRASHER_PATH, type.c_str(), nullptr); err(1, "exec failed"); - } + }); } void CrasherTest::FinishCrasher() { @@ -379,3 +388,20 @@ TEST_F(CrasherTest, backtrace) { 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_F(CrasherTest, PR_SET_DUMPABLE_0_crash) { + StartProcess([]() { + prctl(PR_SET_DUMPABLE, 0); + volatile char* null = static_cast(nullptr); + *null = '\0'; + }); + AssertDeath(SIGSEGV); +} + +TEST_F(CrasherTest, PR_SET_DUMPABLE_0_raise) { + StartProcess([]() { + prctl(PR_SET_DUMPABLE, 0); + raise(SIGUSR1); + }); + AssertDeath(SIGUSR1); +} diff --git a/debuggerd/handler/debuggerd_handler.cpp b/debuggerd/handler/debuggerd_handler.cpp index c031046f2..8ea06c0a3 100644 --- a/debuggerd/handler/debuggerd_handler.cpp +++ b/debuggerd/handler/debuggerd_handler.cpp @@ -63,6 +63,9 @@ static debuggerd_callbacks_t g_callbacks; +// Mutex to ensure only one crashing thread dumps itself. +static pthread_mutex_t crash_mutex = PTHREAD_MUTEX_INITIALIZER; + // Don't use __libc_fatal because it exits via abort, which might put us back into a signal handler. #define fatal(...) \ do { \ @@ -153,7 +156,7 @@ static bool have_siginfo(int signum) { } struct debugger_thread_info { - bool crash_dump_started = false; + bool crash_dump_started; pid_t crashing_tid; pid_t pseudothread_tid; int signal_number; @@ -233,12 +236,37 @@ static int debuggerd_dispatch_pseudothread(void* arg) { return 0; } +static void resend_signal(siginfo_t* info) { + // Signals can either be fatal or nonfatal. + // For fatal signals, crash_dump will send us the signal we crashed with + // before resuming us, so that processes using waitpid on us will see that we + // exited with the correct exit status (e.g. so that sh will report + // "Segmentation fault" instead of "Killed"). For this to work, we need + // to deregister our signal handler for that signal before continuing. + if (info->si_signo != DEBUGGER_SIGNAL) { + signal(info->si_signo, SIG_DFL); + } + + // We need to return from our signal handler so that crash_dump can see the + // signal via ptrace and dump the thread that crashed. However, returning + // does not guarantee that the signal will be thrown again, even for SIGSEGV + // and friends, since the signal could have been sent manually. We blocked + // all signals when registering the handler, so resending the signal (using + // rt_tgsigqueueinfo(2) to preserve SA_SIGINFO) will cause it to be delivered + // when our signal handler returns. + int rc = syscall(SYS_rt_tgsigqueueinfo, getpid(), gettid(), info->si_signo, info); + if (rc != 0) { + fatal("failed to resend signal during crash: %s", strerror(errno)); + } + + if (info->si_signo == DEBUGGER_SIGNAL) { + pthread_mutex_unlock(&crash_mutex); + } +} + // Handler that does crash dumping by forking and doing the processing in the child. // Do this by ptracing the relevant thread, and then execing debuggerd to do the actual dump. static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void*) { - // Mutex to prevent multiple crashing threads from trying to talk - // to debuggerd at the same time. - static pthread_mutex_t crash_mutex = PTHREAD_MUTEX_INITIALIZER; int ret = pthread_mutex_lock(&crash_mutex); if (ret != 0) { __libc_format_log(ANDROID_LOG_INFO, "libc", "pthread_mutex_lock failed: %s", strerror(ret)); @@ -251,14 +279,28 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void*) info = nullptr; } + struct siginfo si = {}; + if (!info) { + memset(&si, 0, sizeof(si)); + si.si_signo = signal_number; + si.si_code = SI_USER; + si.si_pid = getpid(); + si.si_uid = getuid(); + info = &si; + } else if (info->si_code >= 0 || info->si_code == SI_TKILL) { + // rt_tgsigqueueinfo(2)'s documentation appears to be incorrect on kernels + // that contain commit 66dd34a (3.9+). The manpage claims to only allow + // negative si_code values that are not SI_TKILL, but 66dd34a changed the + // check to allow all si_code values in calls coming from inside the house. + } + log_signal_summary(signal_number, info); if (prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) == 0) { - // process has disabled core dumps and PTRACE_ATTACH, and does not want to be dumped. - // Honor that intention by not connecting to debuggerd and asking it to dump our internal state. + // The process has disabled core dumps and PTRACE_ATTACH, and does not want to be dumped. __libc_format_log(ANDROID_LOG_INFO, "libc", "Suppressing debuggerd output because prctl(PR_GET_DUMPABLE)==0"); - pthread_mutex_unlock(&crash_mutex); + resend_signal(info); return; } @@ -266,8 +308,13 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void*) if (g_callbacks.get_abort_message) { abort_message = g_callbacks.get_abort_message(); } + // Populate si_value with the abort message address, if found. + if (abort_message) { + info->si_value.sival_ptr = abort_message; + } debugger_thread_info thread_info = { + .crash_dump_started = false, .pseudothread_tid = -1, .crashing_tid = gettid(), .signal_number = signal_number, @@ -299,42 +346,7 @@ static void debuggerd_signal_handler(int signal_number, siginfo_t* info, void*) signal(signal_number, SIG_DFL); } - // We need to return from the signal handler so that debuggerd can dump the - // thread that crashed, but returning here does not guarantee that the signal - // will be thrown again, even for SIGSEGV and friends, since the signal could - // have been sent manually. Resend the signal with rt_tgsigqueueinfo(2) to - // preserve the SA_SIGINFO contents. - struct siginfo si; - if (!info) { - memset(&si, 0, sizeof(si)); - si.si_code = SI_USER; - si.si_pid = getpid(); - si.si_uid = getuid(); - info = &si; - } else if (info->si_code >= 0 || info->si_code == SI_TKILL) { - // rt_tgsigqueueinfo(2)'s documentation appears to be incorrect on kernels - // that contain commit 66dd34a (3.9+). The manpage claims to only allow - // negative si_code values that are not SI_TKILL, but 66dd34a changed the - // check to allow all si_code values in calls coming from inside the house. - } - - // Populate si_value with the abort message address, if found. - if (abort_message) { - info->si_value.sival_ptr = abort_message; - } - - // Only resend the signal if we know that either crash_dump has ptraced us or - // the signal was fatal. - if (thread_info.crash_dump_started || signal_number != DEBUGGER_SIGNAL) { - int rc = syscall(SYS_rt_tgsigqueueinfo, getpid(), gettid(), signal_number, info); - if (rc != 0) { - fatal("failed to resend signal during crash: %s", strerror(errno)); - } - } - - if (signal_number == DEBUGGER_SIGNAL) { - pthread_mutex_unlock(&crash_mutex); - } + resend_signal(info); } void debuggerd_init(debuggerd_callbacks_t* callbacks) {