From 7c6e3133f57b6c908e211c0013fcb68d5a44d919 Mon Sep 17 00:00:00 2001
From: Josh Gao <jmgao@google.com>
Date: Sun, 22 Jan 2017 17:59:02 -0800
Subject: [PATCH] crash_dump: set a watchdog timer.

PTRACE_DETACH is only necessary if the process is in group-stop state,
the tracer exiting is sufficient to detach and resume tracees.

Using this, set a 5 second timer with alarm(2) that just kills us, to
avoid leaving processes stopped.

Bug: http://b/34472671
Test: debuggerd_test
Test: crasher + manually inserting a 10 second sleep into crash_dump
Change-Id: Iacaa796f79037aa1585f3f2159abe45ef0069311
---
 debuggerd/crash_dump.cpp     | 55 ++++++++++++++++++------------------
 debuggerd/debuggerd_test.cpp | 15 ++--------
 2 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/debuggerd/crash_dump.cpp b/debuggerd/crash_dump.cpp
index 1af00b487..cf6a715a4 100644
--- a/debuggerd/crash_dump.cpp
+++ b/debuggerd/crash_dump.cpp
@@ -257,6 +257,9 @@ int main(int argc, char** argv) {
     exit(0);
   }
 
+  // Die if we take too long.
+  alarm(20);
+
   check_process(target_proc_fd, target);
 
   std::string attach_error;
@@ -316,10 +319,9 @@ int main(int argc, char** argv) {
   // Now that we have the signal that kicked things off, attach all of the
   // sibling threads, and then proceed.
   bool fatal_signal = signo != DEBUGGER_SIGNAL;
-  int resume_signal = fatal_signal ? signo : 0;
   std::set<pid_t> siblings;
   std::set<pid_t> attached_siblings;
-  if (resume_signal == 0) {
+  if (fatal_signal) {
     if (!android::procinfo::GetProcessTids(target, &siblings)) {
       PLOG(FATAL) << "failed to get process siblings";
     }
@@ -352,40 +354,39 @@ int main(int argc, char** argv) {
                       attached_siblings, abort_address, fatal_signal ? &amfd_data : nullptr);
   }
 
+  // We don't actually need to PTRACE_DETACH, as long as our tracees aren't in
+  // group-stop state, which is true as long as no stopping signals are sent.
+
   bool wait_for_gdb = android::base::GetBoolProperty("debug.debuggerd.wait_for_gdb", false);
-  if (wait_for_gdb) {
+  if (!fatal_signal || siginfo.si_code == SI_USER) {
     // Don't wait_for_gdb when the process didn't actually crash.
-    if (!fatal_signal) {
-      wait_for_gdb = false;
-    } else {
-      // Use ALOGI to line up with output from engrave_tombstone.
-      ALOGI(
-        "***********************************************************\n"
-        "* Process %d has been suspended while crashing.\n"
-        "* To attach gdbserver and start gdb, run this on the host:\n"
-        "*\n"
-        "*     gdbclient.py -p %d\n"
-        "*\n"
-        "***********************************************************",
-        target, main_tid);
-    }
+    wait_for_gdb = false;
   }
 
-  for (pid_t tid : attached_siblings) {
-    // Don't send the signal to sibling threads.
-    if (ptrace(PTRACE_DETACH, tid, 0, wait_for_gdb ? SIGSTOP : 0) != 0) {
-      PLOG(ERROR) << "ptrace detach from " << tid << " failed";
+  // If the process crashed or we need to send it SIGSTOP for wait_for_gdb,
+  // get it in a state where it can receive signals, and then send the relevant
+  // signal.
+  if (wait_for_gdb || fatal_signal) {
+    if (ptrace(PTRACE_INTERRUPT, main_tid, 0, 0) != 0) {
+      PLOG(ERROR) << "failed to use PTRACE_INTERRUPT on " << main_tid;
     }
-  }
 
-  if (ptrace(PTRACE_DETACH, main_tid, 0, wait_for_gdb ? SIGSTOP : resume_signal)) {
-    PLOG(ERROR) << "ptrace detach from main thread " << main_tid << " failed";
+    if (tgkill(target, main_tid, wait_for_gdb ? SIGSTOP : signo) != 0) {
+      PLOG(ERROR) << "failed to resend signal " << signo << " to " << main_tid;
+    }
   }
 
   if (wait_for_gdb) {
-    if (tgkill(target, main_tid, resume_signal) != 0) {
-      PLOG(ERROR) << "failed to resend signal to process " << target;
-    }
+    // Use ALOGI to line up with output from engrave_tombstone.
+    ALOGI(
+      "***********************************************************\n"
+      "* Process %d has been suspended while crashing.\n"
+      "* To attach gdbserver and start gdb, run this on the host:\n"
+      "*\n"
+      "*     gdbclient.py -p %d\n"
+      "*\n"
+      "***********************************************************",
+      target, main_tid);
   }
 
   if (fatal_signal) {
diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp
index f51b5f253..3b2419365 100644
--- a/debuggerd/debuggerd_test.cpp
+++ b/debuggerd/debuggerd_test.cpp
@@ -340,24 +340,15 @@ TEST_F(CrasherTest, wait_for_gdb) {
   AssertDeath(SIGABRT);
 }
 
+// wait_for_gdb shouldn't trigger on manually sent signals.
 TEST_F(CrasherTest, wait_for_gdb_signal) {
   if (!android::base::SetProperty(kWaitForGdbKey, "1")) {
     FAIL() << "failed to enable wait_for_gdb";
   }
 
   StartCrasher("abort");
-  ASSERT_EQ(0, kill(crasher_pid, SIGABRT)) << strerror(errno);
-
-  std::this_thread::sleep_for(500ms);
-
-  int status;
-  ASSERT_EQ(crasher_pid, (TIMEOUT(1, waitpid(crasher_pid, &status, WUNTRACED))));
-  ASSERT_TRUE(WIFSTOPPED(status));
-  ASSERT_EQ(SIGSTOP, WSTOPSIG(status));
-
-  ASSERT_EQ(0, kill(crasher_pid, SIGCONT)) << strerror(errno);
-
-  AssertDeath(SIGABRT);
+  ASSERT_EQ(0, kill(crasher_pid, SIGSEGV)) << strerror(errno);
+  AssertDeath(SIGSEGV);
 }
 
 TEST_F(CrasherTest, backtrace) {