From e740250b9d966c7eb9a7959a47526906780d7715 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 1 Jun 2017 11:55:25 -0700 Subject: [PATCH 1/3] crash_dump: clear the signal mask. crash_dump inherits its signal mask from the thread that forked it, which always has all of its signals blocked, now that sigchain respects sa_mask. Manually clear the signal mask, and reduce the timeout to a still-generous 2 seconds. Bug: http://b/38427757 Test: manually inserted sleep in crash_dump Change-Id: If1c9adb68777b71fb19d9b0f47d6998733ed8f52 --- debuggerd/crash_dump.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index cc7824219..fbf56767c 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -207,9 +207,14 @@ int main(int argc, char** argv) { action.sa_handler = signal_handler; debuggerd_register_handlers(&action); + sigset_t mask; + sigemptyset(&mask); + if (sigprocmask(SIG_SETMASK, &mask, nullptr) != 0) { + PLOG(FATAL) << "failed to set signal mask"; + } + if (argc != 4) { LOG(FATAL) << "Wrong number of args: " << argc << " (expected 4)"; - return 1; } pid_t main_tid; @@ -264,7 +269,7 @@ int main(int argc, char** argv) { } // Die if we take too long. - alarm(20); + alarm(2); std::string attach_error; From b0e51e388b8c952e28280f0e7035dc18fb9aa4e5 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 1 Jun 2017 12:08:10 -0700 Subject: [PATCH 2/3] crash_dump: don't notify ActivityManager if it crashed. Bug: http://b/38427757 Test: killall -ABRT system_server, plus added logging Change-Id: Ic15e0b0870b1ec08a2f165ad0e5356afed02eece --- debuggerd/crash_dump.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp index fbf56767c..558bc721a 100644 --- a/debuggerd/crash_dump.cpp +++ b/debuggerd/crash_dump.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include "backtrace.h" @@ -99,8 +100,9 @@ static bool ptrace_seize_thread(int pid_proc_fd, pid_t tid, std::string* error) return true; } -static bool activity_manager_notify(int pid, int signal, const std::string& amfd_data) { - android::base::unique_fd amfd(socket_local_client("/data/system/ndebugsocket", ANDROID_SOCKET_NAMESPACE_FILESYSTEM, SOCK_STREAM)); +static bool activity_manager_notify(pid_t pid, int signal, const std::string& amfd_data) { + android::base::unique_fd amfd(socket_local_client( + "/data/system/ndebugsocket", ANDROID_SOCKET_NAMESPACE_FILESYSTEM, SOCK_STREAM)); if (amfd.get() == -1) { PLOG(ERROR) << "unable to connect to activity manager"; return false; @@ -413,7 +415,10 @@ int main(int argc, char** argv) { } if (fatal_signal) { - activity_manager_notify(target, signo, amfd_data); + // Don't try to notify ActivityManager if it just crashed, or we might hang until timeout. + if (target_info.name != "system_server" || target_info.uid != AID_SYSTEM) { + activity_manager_notify(target, signo, amfd_data); + } } // Close stdout before we notify tombstoned of completion. From 5675f3c321b7c6a9bbb2a7aacf18a01126f2f2cb Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 1 Jun 2017 12:19:53 -0700 Subject: [PATCH 3/3] debuggerd_client: increase pipe buffer size to max. If a process tries to dump itself (e.g. system_server during ANRs), crash_dump will block trying to write to its pipe if it's not sufficiently large. Increase the pipe size to the max, and add a test to make sure that it's always at least 1MB (the default value). Bug: http://b/38427757 Test: debuggerd_test Change-Id: Iddb0cb1e5ce9e687efa9e94c2748a1edfe09f119 --- debuggerd/client/debuggerd_client.cpp | 16 ++++++++++++++++ debuggerd/debuggerd_test.cpp | 2 ++ 2 files changed, 18 insertions(+) diff --git a/debuggerd/client/debuggerd_client.cpp b/debuggerd/client/debuggerd_client.cpp index c357fd6d6..cb7cbbe0f 100644 --- a/debuggerd/client/debuggerd_client.cpp +++ b/debuggerd/client/debuggerd_client.cpp @@ -28,7 +28,9 @@ #include #include +#include #include +#include #include #include @@ -117,6 +119,20 @@ bool debuggerd_trigger_dump(pid_t pid, DebuggerdDumpType dump_type, unsigned int return false; } + std::string pipe_size_str; + int pipe_buffer_size = 1024 * 1024; + if (android::base::ReadFileToString("/proc/sys/fs/pipe-max-size", &pipe_size_str)) { + pipe_size_str = android::base::Trim(pipe_size_str); + + if (!android::base::ParseInt(pipe_size_str.c_str(), &pipe_buffer_size, 0)) { + LOG(FATAL) << "failed to parse pipe max size '" << pipe_size_str << "'"; + } + } + + if (fcntl(pipe_read.get(), F_SETPIPE_SZ, pipe_buffer_size) != pipe_buffer_size) { + PLOG(ERROR) << "failed to set pipe buffer size"; + } + if (send_fd(set_timeout(sockfd), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) { PLOG(ERROR) << "libdebuggerd_client: failed to send output fd to tombstoned"; return false; diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 8c00b76a0..9008b95ac 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -119,6 +119,8 @@ static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, uniq FAIL() << "failed to set pipe size: " << strerror(errno); } + ASSERT_GE(pipe_buffer_size, 1024 * 1024); + if (send_fd(intercept_fd->get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) { FAIL() << "failed to send output fd to tombstoned: " << strerror(errno); }