Merge "debuggerd: fix several bugs caused by fork/setuid change."

am: 1bf7000033

* commit '1bf700003368e8b68b34a0f9968ae5afd25dd4f2':
  debuggerd: fix several bugs caused by fork/setuid change.

7 files changed, 251 insertions, 275 deletions

diff --git a/debuggerd/backtrace.cpp b/debuggerd/backtrace.cpp
index b46f8f443..b6916e5b4 100644
--- a/debuggerd/backtrace.cpp
+++ b/debuggerd/backtrace.cpp
@@ -67,8 +67,7 @@ static void dump_process_footer(log_t* log, pid_t pid) {
   _LOG(log, logtype::BACKTRACE, "\n----- end %d -----\n", pid);
 }
 
-static void dump_thread(
-    log_t* log, pid_t tid, bool attached, bool* detach_failed, int* total_sleep_time_usec) {
+static void dump_thread(log_t* log, BacktraceMap* map, pid_t pid, pid_t tid) {
   char path[PATH_MAX];
   char threadnamebuf[1024];
   char* threadname = NULL;
@@ -88,56 +87,25 @@ static void dump_thread(
 
   _LOG(log, logtype::BACKTRACE, "\n\"%s\" sysTid=%d\n", threadname ? threadname : "<unknown>", tid);
 
-  if (!attached && ptrace(PTRACE_ATTACH, tid, 0, 0) < 0) {
-    _LOG(log, logtype::BACKTRACE, "Could not attach to thread: %s\n", strerror(errno));
-    return;
-  }
-
-  if (!attached && wait_for_sigstop(tid, total_sleep_time_usec, detach_failed) == -1) {
-    return;
-  }
-
-  std::unique_ptr<Backtrace> backtrace(Backtrace::Create(tid, BACKTRACE_CURRENT_THREAD));
+  std::unique_ptr<Backtrace> backtrace(Backtrace::Create(pid, tid, map));
   if (backtrace->Unwind(0)) {
     dump_backtrace_to_log(backtrace.get(), log, "  ");
   } else {
     ALOGE("Unwind failed: tid = %d", tid);
   }
-
-  if (!attached && ptrace(PTRACE_DETACH, tid, 0, 0) != 0) {
-    ALOGE("ptrace detach from %d failed: %s\n", tid, strerror(errno));
-    *detach_failed = true;
-  }
 }
 
-void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
-                    int* total_sleep_time_usec) {
+void dump_backtrace(int fd, int amfd, BacktraceMap* map, pid_t pid, pid_t tid,
+                    const std::set<pid_t>& siblings) {
   log_t log;
   log.tfd = fd;
   log.amfd = amfd;
 
   dump_process_header(&log, pid);
-  dump_thread(&log, tid, true, detach_failed, total_sleep_time_usec);
-
-  char task_path[64];
-  snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
-  DIR* d = opendir(task_path);
-  if (d != NULL) {
-    struct dirent* de = NULL;
-    while ((de = readdir(d)) != NULL) {
-      if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) {
-        continue;
-      }
+  dump_thread(&log, map, pid, tid);
 
-      char* end;
-      pid_t new_tid = strtoul(de->d_name, &end, 10);
-      if (*end || new_tid == tid) {
-        continue;
-      }
-
-      dump_thread(&log, new_tid, false, detach_failed, total_sleep_time_usec);
-    }
-    closedir(d);
+  for (pid_t sibling : siblings) {
+    dump_thread(&log, map, pid, sibling);
   }
 
   dump_process_footer(&log, pid);
diff --git a/debuggerd/backtrace.h b/debuggerd/backtrace.h
index da14cd466..98c433b6b 100644
--- a/debuggerd/backtrace.h
+++ b/debuggerd/backtrace.h
@@ -19,14 +19,17 @@
 
 #include <sys/types.h>
 
+#include <set>
+
 #include "utility.h"
 
 class Backtrace;
+class BacktraceMap;
 
 // Dumps a backtrace using a format similar to what Dalvik uses so that the result
 // can be intermixed in a bug report.
-void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
-                    int* total_sleep_time_usec);
+void dump_backtrace(int fd, int amfd, BacktraceMap* map, pid_t pid, pid_t tid,
+                    const std::set<pid_t>& siblings);
 
 /* Dumps the backtrace in the backtrace data structure to the log. */
 void dump_backtrace_to_log(Backtrace* backtrace, log_t* log, const char* prefix);
diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp
index 58b629b15..a8d19fc12 100644
--- a/debuggerd/debuggerd.cpp
+++ b/debuggerd/debuggerd.cpp
@@ -14,14 +14,14 @@
  * limitations under the License.
  */
 
-#include <stdio.h>
+#include <dirent.h>
 #include <errno.h>
-#include <signal.h>
+#include <fcntl.h>
 #include <pthread.h>
+#include <signal.h>
 #include <stdarg.h>
-#include <fcntl.h>
+#include <stdio.h>
 #include <sys/types.h>
-#include <dirent.h>
 #include <time.h>
 
 #include <elf.h>
@@ -31,6 +31,8 @@
 #include <sys/stat.h>
 #include <sys/wait.h>
 
+#include <set>
+
 #include <selinux/android.h>
 
 #include <log/logger.h>
@@ -57,6 +59,8 @@
 #define SOCKET_NAME DEBUGGER_SOCKET_NAME
 #endif
 
+extern "C" int tgkill(int tgid, int tid, int sig);
+
 struct debugger_request_t {
   debugger_action_t action;
   pid_t pid, tid;
@@ -335,6 +339,121 @@ static void redirect_to_32(int fd, debugger_request_t* request) {
 }
 #endif
 
+static void ptrace_siblings(pid_t pid, pid_t main_tid, std::set<pid_t>& tids) {
+  char task_path[64];
+
+  snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
+
+  std::unique_ptr<DIR, int (*)(DIR*)> d(opendir(task_path), closedir);
+
+  // Bail early if the task directory cannot be opened.
+  if (!d) {
+    ALOGE("debuggerd: failed to open /proc/%d/task: %s", pid, strerror(errno));
+    return;
+  }
+
+  struct dirent* de;
+  while ((de = readdir(d.get())) != NULL) {
+    // Ignore "." and "..".
+    if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) {
+      continue;
+    }
+
+    char* end;
+    pid_t tid = strtoul(de->d_name, &end, 10);
+    if (*end) {
+      continue;
+    }
+
+    if (tid == main_tid) {
+      continue;
+    }
+
+    if (ptrace(PTRACE_ATTACH, tid, 0, 0) < 0) {
+      ALOGE("debuggerd: ptrace attach to %d failed: %s", tid, strerror(errno));
+      continue;
+    }
+
+    tids.insert(tid);
+  }
+}
+
+static bool perform_dump(const debugger_request_t& request, int fd, int tombstone_fd,
+                         BacktraceMap* backtrace_map, const std::set<pid_t>& siblings) {
+  if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) {
+    ALOGE("debuggerd: failed to respond to client: %s\n", strerror(errno));
+    return false;
+  }
+
+  int total_sleep_time_usec = 0;
+  while (true) {
+    int signal = wait_for_signal(request.tid, &total_sleep_time_usec);
+    switch (signal) {
+      case -1:
+        ALOGE("debuggerd: timed out waiting for signal");
+        return false;
+
+      case SIGSTOP:
+        if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) {
+          ALOGV("debuggerd: stopped -- dumping to tombstone");
+          engrave_tombstone(tombstone_fd, backtrace_map, request.pid, request.tid, siblings, signal,
+                            request.original_si_code, request.abort_msg_address);
+        } else if (request.action == DEBUGGER_ACTION_DUMP_BACKTRACE) {
+          ALOGV("debuggerd: stopped -- dumping to fd");
+          dump_backtrace(fd, -1, backtrace_map, request.pid, request.tid, siblings);
+        } else {
+          ALOGV("debuggerd: stopped -- continuing");
+          if (ptrace(PTRACE_CONT, request.tid, 0, 0) != 0) {
+            ALOGE("debuggerd: ptrace continue failed: %s", strerror(errno));
+            return false;
+          }
+          continue;  // loop again
+        }
+        break;
+
+      case SIGABRT:
+      case SIGBUS:
+      case SIGFPE:
+      case SIGILL:
+      case SIGSEGV:
+#ifdef SIGSTKFLT
+      case SIGSTKFLT:
+#endif
+      case SIGTRAP:
+        ALOGV("stopped -- fatal signal\n");
+        // Send a SIGSTOP to the process to make all of
+        // the non-signaled threads stop moving.  Without
+        // this we get a lot of "ptrace detach failed:
+        // No such process".
+        kill(request.pid, SIGSTOP);
+        engrave_tombstone(tombstone_fd, backtrace_map, request.pid, request.tid, siblings, signal,
+                          request.original_si_code, request.abort_msg_address);
+        break;
+
+      default:
+        ALOGE("debuggerd: process stopped due to unexpected signal %d\n", signal);
+        break;
+    }
+    break;
+  }
+
+  return true;
+}
+
+static bool drop_privileges() {
+  if (setresgid(AID_DEBUGGERD, AID_DEBUGGERD, AID_DEBUGGERD) != 0) {
+    ALOGE("debuggerd: failed to setresgid");
+    return false;
+  }
+
+  if (setresuid(AID_DEBUGGERD, AID_DEBUGGERD, AID_DEBUGGERD) != 0) {
+    ALOGE("debuggerd: failed to setresuid");
+    return false;
+  }
+
+  return true;
+}
+
 static void handle_request(int fd) {
   ALOGV("handle_request(%d)\n", fd);
 
@@ -405,117 +524,63 @@ static void handle_request(int fd) {
   // ensure that it can run as soon as we call PTRACE_CONT below.
   // See details in bionic/libc/linker/debugger.c, in function
   // debugger_signal_handler().
-  if (ptrace(PTRACE_ATTACH, request.tid, 0, 0)) {
-    ALOGE("debuggerd: ptrace attach failed: %s\n", strerror(errno));
-    exit(1);
-  }
-
-  // Generate the backtrace map before dropping privileges.
-  std::unique_ptr<BacktraceMap> backtrace_map(BacktraceMap::Create(request.pid));
-
-  // Now that we've done everything that requires privileges, we can drop them.
-  if (setresgid(AID_DEBUGGERD, AID_DEBUGGERD, AID_DEBUGGERD) != 0) {
-    ALOGE("debuggerd: failed to setresgid");
-    exit(1);
-  }
 
-  if (setresuid(AID_DEBUGGERD, AID_DEBUGGERD, AID_DEBUGGERD) != 0) {
-    ALOGE("debuggerd: failed to setresuid");
+  // Attach to the target process.
+  if (ptrace(PTRACE_ATTACH, request.tid, 0, 0) != 0) {
+    ALOGE("debuggerd: ptrace attach failed: %s", strerror(errno));
     exit(1);
   }
 
-  bool detach_failed = false;
-  bool tid_unresponsive = false;
+  // Don't attach to the sibling threads if we want to attach gdb.
+  // Supposedly, it makes the process less reliable.
   bool attach_gdb = should_attach_gdb(&request);
-  if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) {
-    ALOGE("debuggerd: failed to respond to client: %s\n", strerror(errno));
-    exit(1);
+  std::set<pid_t> siblings;
+  if (!attach_gdb) {
+    ptrace_siblings(request.pid, request.tid, siblings);
   }
 
-  int total_sleep_time_usec = 0;
-  while (true) {
-    int signal = wait_for_sigstop(request.tid, &total_sleep_time_usec, &detach_failed);
-    if (signal == -1) {
-      tid_unresponsive = true;
-      break;
-    }
+  // Generate the backtrace map before dropping privileges.
+  std::unique_ptr<BacktraceMap> backtrace_map(BacktraceMap::Create(request.pid));
 
-    switch (signal) {
-      case SIGSTOP:
-        if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) {
-          ALOGV("stopped -- dumping to tombstone\n");
-          engrave_tombstone(tombstone_fd, backtrace_map.get(), request.pid, request.tid, signal,
-                            request.original_si_code, request.abort_msg_address, true,
-                            &detach_failed, &total_sleep_time_usec);
-        } else if (request.action == DEBUGGER_ACTION_DUMP_BACKTRACE) {
-          ALOGV("stopped -- dumping to fd\n");
-          dump_backtrace(fd, -1, request.pid, request.tid, &detach_failed, &total_sleep_time_usec);
-        } else {
-          ALOGV("stopped -- continuing\n");
-          status = ptrace(PTRACE_CONT, request.tid, 0, 0);
-          if (status) {
-            ALOGE("debuggerd: ptrace continue failed: %s\n", strerror(errno));
-          }
-          continue;  // loop again
-        }
-        break;
+  bool succeeded = false;
 
-      case SIGABRT:
-      case SIGBUS:
-      case SIGFPE:
-      case SIGILL:
-      case SIGSEGV:
-#ifdef SIGSTKFLT
-      case SIGSTKFLT:
-#endif
-      case SIGTRAP:
-        ALOGV("stopped -- fatal signal\n");
-        // Send a SIGSTOP to the process to make all of
-        // the non-signaled threads stop moving.  Without
-        // this we get a lot of "ptrace detach failed:
-        // No such process".
-        kill(request.pid, SIGSTOP);
-        // don't dump sibling threads when attaching to GDB because it
-        // makes the process less reliable, apparently...
-        engrave_tombstone(tombstone_fd, backtrace_map.get(), request.pid, request.tid, signal,
-                          request.original_si_code, request.abort_msg_address, !attach_gdb,
-                          &detach_failed, &total_sleep_time_usec);
-        break;
+  // Now that we've done everything that requires privileges, we can drop them.
+  if (drop_privileges()) {
+    succeeded = perform_dump(request, fd, tombstone_fd, backtrace_map.get(), siblings);
+    if (succeeded) {
+      if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) {
+        if (!tombstone_path.empty()) {
+          write(fd, tombstone_path.c_str(), tombstone_path.length());
+        }
+      }
+    }
 
-      default:
-        ALOGE("debuggerd: process stopped due to unexpected signal %d\n", signal);
-        break;
+    if (attach_gdb) {
+      // Stop the process so we can debug.
+      tgkill(request.pid, request.tid, SIGSTOP);
     }
-    break;
   }
 
-  if (request.action == DEBUGGER_ACTION_DUMP_TOMBSTONE) {
-    if (!tombstone_path.empty()) {
-      write(fd, tombstone_path.c_str(), tombstone_path.length());
-    }
+  if (ptrace(PTRACE_DETACH, request.tid, 0, 0) != 0) {
+    ALOGE("debuggerd: ptrace detach from %d failed: %s", request.tid, strerror(errno));
   }
 
-  if (!tid_unresponsive) {
-    ALOGV("detaching");
-    if (attach_gdb) {
-      // stop the process so we can debug
-      kill(request.pid, SIGSTOP);
-    }
-    if (ptrace(PTRACE_DETACH, request.tid, 0, 0)) {
-      ALOGE("debuggerd: ptrace detach from %d failed: %s", request.tid, strerror(errno));
-      detach_failed = true;
-    } else if (attach_gdb) {
-      // if debug.db.uid is set, its value indicates if we should wait
-      // for user action for the crashing process.
-      // in this case, we log a message and turn the debug LED on
-      // waiting for a gdb connection (for instance)
-      wait_for_user_action(request);
-    }
+  for (pid_t sibling : siblings) {
+    ptrace(PTRACE_DETACH, sibling, 0, 0);
+  }
+
+  if (succeeded && attach_gdb) {
+    // if debug.debuggerd.wait_for_gdb is set, its value indicates if we should wait
+    // for user action for the crashing process.
+    // in this case, we log a message and turn the debug LED on
+    // waiting for a gdb connection (for instance)
+    wait_for_user_action(request);
   }
 
-  // Resume the stopped process so it can crash in peace, and exit.
+  // Resume the stopped process.
   kill(request.pid, SIGCONT);
-  exit(0);
+
+  exit(!succeeded);
 }
 
 static int do_server() {
diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp
index b2f203d2b..dda6677a9 100644
--- a/debuggerd/tombstone.cpp
+++ b/debuggerd/tombstone.cpp
@@ -328,6 +328,33 @@ static std::string get_addr_string(uintptr_t addr) {
   return addr_str;
 }
 
+static void dump_abort_message(Backtrace* backtrace, log_t* log, uintptr_t address) {
+  if (address == 0) {
+    return;
+  }
+
+  address += sizeof(size_t);  // Skip the buffer length.
+
+  char msg[512];
+  memset(msg, 0, sizeof(msg));
+  char* p = &msg[0];
+  while (p < &msg[sizeof(msg)]) {
+    word_t data;
+    size_t len = sizeof(word_t);
+    if (!backtrace->ReadWord(address, &data)) {
+      break;
+    }
+    address += sizeof(word_t);
+
+    while (len > 0 && (*p++ = (data >> (sizeof(word_t) - len) * 8) & 0xff) != 0) {
+      len--;
+    }
+  }
+  msg[sizeof(msg) - 1] = '\0';
+
+  _LOG(log, logtype::HEADER, "Abort message: '%s'\n", msg);
+}
+
 static void dump_all_maps(Backtrace* backtrace, BacktraceMap* map, log_t* log, pid_t tid) {
   bool print_fault_address_marker = false;
   uintptr_t addr = 0;
@@ -416,67 +443,37 @@ static void dump_backtrace_and_stack(Backtrace* backtrace, log_t* log) {
   }
 }
 
-// Return true if some thread is not detached cleanly
-static bool dump_sibling_thread_report(
-    log_t* log, pid_t pid, pid_t tid, int* total_sleep_time_usec, BacktraceMap* map) {
-  char task_path[64];
-
-  snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
-
-  DIR* d = opendir(task_path);
-  // Bail early if the task directory cannot be opened
-  if (d == NULL) {
-    ALOGE("Cannot open /proc/%d/task\n", pid);
-    return false;
-  }
-
-  bool detach_failed = false;
-  struct dirent* de;
-  while ((de = readdir(d)) != NULL) {
-    // Ignore "." and ".."
-    if (!strcmp(de->d_name, ".") || !strcmp(de->d_name, "..")) {
-      continue;
-    }
-
-    // The main thread at fault has been handled individually
-    char* end;
-    pid_t new_tid = strtoul(de->d_name, &end, 10);
-    if (*end || new_tid == tid) {
-      continue;
-    }
-
-    // Skip this thread if cannot ptrace it
-    if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) {
-      ALOGE("ptrace attach to %d failed: %s\n", new_tid, strerror(errno));
-      continue;
-    }
-
-    if (wait_for_sigstop(new_tid, total_sleep_time_usec, &detach_failed) == -1) {
-      continue;
-    }
-
-    log->current_tid = new_tid;
+static void dump_thread(log_t* log, pid_t pid, pid_t tid, BacktraceMap* map, int signal,
+                        int si_code, uintptr_t abort_msg_address, bool primary_thread) {
+  log->current_tid = tid;
+  if (!primary_thread) {
     _LOG(log, logtype::THREAD, "--- --- --- --- --- --- --- --- --- --- --- --- --- --- --- ---\n");
-    dump_thread_info(log, pid, new_tid);
+  }
+  dump_thread_info(log, pid, tid);
 
-    dump_registers(log, new_tid);
-    std::unique_ptr<Backtrace> backtrace(Backtrace::Create(pid, new_tid, map));
-    if (backtrace->Unwind(0)) {
-      dump_backtrace_and_stack(backtrace.get(), log);
-    } else {
-      ALOGE("Unwind of sibling failed: pid = %d, tid = %d", pid, new_tid);
-    }
+  if (signal) {
+    dump_signal_info(log, tid, signal, si_code);
+  }
 
-    log->current_tid = log->crashed_tid;
+  std::unique_ptr<Backtrace> backtrace(Backtrace::Create(pid, tid, map));
+  if (primary_thread) {
+    dump_abort_message(backtrace.get(), log, abort_msg_address);
+  }
+  dump_registers(log, tid);
+  if (backtrace->Unwind(0)) {
+    dump_backtrace_and_stack(backtrace.get(), log);
+  } else {
+    ALOGE("Unwind failed: pid = %d, tid = %d", pid, tid);
+  }
 
-    if (ptrace(PTRACE_DETACH, new_tid, 0, 0) != 0) {
-      ALOGE("ptrace detach from %d failed: %s\n", new_tid, strerror(errno));
-      detach_failed = true;
+  if (primary_thread) {
+    dump_memory_and_code(log, backtrace.get());
+    if (map) {
+      dump_all_maps(backtrace.get(), map, log, tid);
     }
   }
 
-  closedir(d);
-  return detach_failed;
+  log->current_tid = log->crashed_tid;
 }
 
 // Reads the contents of the specified log device, filters out the entries
@@ -605,36 +602,10 @@ static void dump_logs(log_t* log, pid_t pid, unsigned int tail) {
   dump_log_file(log, pid, "main", tail);
 }
 
-static void dump_abort_message(Backtrace* backtrace, log_t* log, uintptr_t address) {
-  if (address == 0) {
-    return;
-  }
-
-  address += sizeof(size_t); // Skip the buffer length.
-
-  char msg[512];
-  memset(msg, 0, sizeof(msg));
-  char* p = &msg[0];
-  while (p < &msg[sizeof(msg)]) {
-    word_t data;
-    size_t len = sizeof(word_t);
-    if (!backtrace->ReadWord(address, &data)) {
-      break;
-    }
-    address += sizeof(word_t);
-
-    while (len > 0 && (*p++ = (data >> (sizeof(word_t) - len) * 8) & 0xff) != 0)
-       len--;
-  }
-  msg[sizeof(msg) - 1] = '\0';
-
-  _LOG(log, logtype::HEADER, "Abort message: '%s'\n", msg);
-}
-
 // Dumps all information about the specified pid to the tombstone.
-static bool dump_crash(log_t* log, BacktraceMap* map, pid_t pid, pid_t tid, int signal, int si_code,
-                       uintptr_t abort_msg_address, bool dump_sibling_threads,
-                       int* total_sleep_time_usec) {
+static void dump_crash(log_t* log, BacktraceMap* map, pid_t pid, pid_t tid,
+                       const std::set<pid_t>& siblings, int signal, int si_code,
+                       uintptr_t abort_msg_address) {
   // don't copy log messages to tombstone unless this is a dev device
   char value[PROPERTY_VALUE_MAX];
   property_get("ro.debuggable", value, "0");
@@ -653,32 +624,15 @@ static bool dump_crash(log_t* log, BacktraceMap* map, pid_t pid, pid_t tid, int
   _LOG(log, logtype::HEADER,
        "*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***\n");
   dump_header_info(log);
-  dump_thread_info(log, pid, tid);
-
-  if (signal) {
-    dump_signal_info(log, tid, signal, si_code);
-  }
-
-  std::unique_ptr<Backtrace> backtrace(Backtrace::Create(pid, tid, map));
-  dump_abort_message(backtrace.get(), log, abort_msg_address);
-  dump_registers(log, tid);
-  if (backtrace->Unwind(0)) {
-    dump_backtrace_and_stack(backtrace.get(), log);
-  } else {
-    ALOGE("Unwind failed: pid = %d, tid = %d", pid, tid);
-  }
-  dump_memory_and_code(log, backtrace.get());
-  if (map) {
-    dump_all_maps(backtrace.get(), map, log, tid);
-  }
-
+  dump_thread(log, pid, tid, map, signal, si_code, abort_msg_address, true);
   if (want_logs) {
     dump_logs(log, pid, 5);
   }
 
-  bool detach_failed = false;
-  if (dump_sibling_threads) {
-    detach_failed = dump_sibling_thread_report(log, pid, tid, total_sleep_time_usec, map);
+  if (!siblings.empty()) {
+    for (pid_t sibling : siblings) {
+      dump_thread(log, pid, sibling, map, 0, 0, 0, false);
+    }
   }
 
   if (want_logs) {
@@ -694,7 +648,7 @@ static bool dump_crash(log_t* log, BacktraceMap* map, pid_t pid, pid_t tid, int
     TEMP_FAILURE_RETRY( read(log->amfd, &eodMarker, 1) );
   }
 
-  return detach_failed;
+  return;
 }
 
 // open_tombstone - find an available tombstone slot, if any, of the
@@ -780,16 +734,15 @@ static int activity_manager_connect() {
   return amfd;
 }
 
-void engrave_tombstone(int tombstone_fd, BacktraceMap* map, pid_t pid, pid_t tid, int signal,
-                       int original_si_code, uintptr_t abort_msg_address, bool dump_sibling_threads,
-                       bool* detach_failed, int* total_sleep_time_usec) {
+void engrave_tombstone(int tombstone_fd, BacktraceMap* map, pid_t pid, pid_t tid,
+                       const std::set<pid_t>& siblings, int signal, int original_si_code,
+                       uintptr_t abort_msg_address) {
   log_t log;
   log.current_tid = tid;
   log.crashed_tid = tid;
 
   if (tombstone_fd < 0) {
     ALOGE("debuggerd: skipping tombstone write, nothing to do.\n");
-    *detach_failed = false;
     return;
   }
 
@@ -798,8 +751,7 @@ void engrave_tombstone(int tombstone_fd, BacktraceMap* map, pid_t pid, pid_t tid
   // being closed.
   int amfd = activity_manager_connect();
   log.amfd = amfd;
-  *detach_failed = dump_crash(&log, map, pid, tid, signal, original_si_code, abort_msg_address,
-                              dump_sibling_threads, total_sleep_time_usec);
+  dump_crash(&log, map, pid, tid, siblings, signal, original_si_code, abort_msg_address);
 
   // This file descriptor can be -1, any error is ignored.
   close(amfd);
diff --git a/debuggerd/tombstone.h b/debuggerd/tombstone.h
index 5f2d23982..2b8b8bed7 100644
--- a/debuggerd/tombstone.h
+++ b/debuggerd/tombstone.h
@@ -20,6 +20,7 @@
 #include <stdbool.h>
 #include <stddef.h>
 #include <sys/types.h>
+#include <set>
 #include <string>
 
 class BacktraceMap;
@@ -30,10 +31,9 @@ class BacktraceMap;
  */
 int open_tombstone(std::string* path);
 
-/* Creates a tombstone file and writes the crash dump to it.
- * Returns the path of the tombstone, which must be freed using free(). */
-void engrave_tombstone(int tombstone_fd, BacktraceMap* map, pid_t pid, pid_t tid, int signal,
-                       int original_si_code, uintptr_t abort_msg_address, bool dump_sibling_threads,
-                       bool* detach_failed, int* total_sleep_time_usec);
+/* Creates a tombstone file and writes the crash dump to it. */
+void engrave_tombstone(int tombstone_fd, BacktraceMap* map, pid_t pid, pid_t tid,
+                       const std::set<pid_t>& siblings, int signal, int original_si_code,
+                       uintptr_t abort_msg_address);
 
 #endif // _DEBUGGERD_TOMBSTONE_H
diff --git a/debuggerd/utility.cpp b/debuggerd/utility.cpp
index ce214f921..cd252ce72 100644
--- a/debuggerd/utility.cpp
+++ b/debuggerd/utility.cpp
@@ -30,8 +30,8 @@
 #include <backtrace/Backtrace.h>
 #include <log/log.h>
 
-const int SLEEP_TIME_USEC = 50000;         // 0.05 seconds
-const int MAX_TOTAL_SLEEP_USEC = 10000000; // 10 seconds
+constexpr int SLEEP_TIME_USEC = 50000;          // 0.05 seconds
+constexpr int MAX_TOTAL_SLEEP_USEC = 10000000;  // 10 seconds
 
 // Whitelist output desired in the logcat output.
 bool is_allowed_in_logcat(enum logtype ltype) {
@@ -78,14 +78,13 @@ void _LOG(log_t* log, enum logtype ltype, const char* fmt, ...) {
   }
 }
 
-int wait_for_sigstop(pid_t tid, int* total_sleep_time_usec, bool* detach_failed) {
-  bool allow_dead_tid = false;
-  for (;;) {
+int wait_for_signal(pid_t tid, int* total_sleep_time_usec) {
+  while (true) {
     int status;
     pid_t n = TEMP_FAILURE_RETRY(waitpid(tid, &status, __WALL | WNOHANG));
     if (n == -1) {
       ALOGE("waitpid failed: tid %d, %s", tid, strerror(errno));
-      break;
+      return -1;
     } else if (n == tid) {
       if (WIFSTOPPED(status)) {
         return WSTOPSIG(status);
@@ -93,29 +92,18 @@ int wait_for_sigstop(pid_t tid, int* total_sleep_time_usec, bool* detach_failed)
         ALOGE("unexpected waitpid response: n=%d, status=%08x\n", n, status);
         // This is the only circumstance under which we can allow a detach
         // to fail with ESRCH, which indicates the tid has exited.
-        allow_dead_tid = true;
-        break;
+        return -1;
       }
     }
 
     if (*total_sleep_time_usec > MAX_TOTAL_SLEEP_USEC) {
       ALOGE("timed out waiting for stop signal: tid=%d", tid);
-      break;
+      return -1;
     }
 
     usleep(SLEEP_TIME_USEC);
     *total_sleep_time_usec += SLEEP_TIME_USEC;
   }
-
-  if (ptrace(PTRACE_DETACH, tid, 0, 0) != 0) {
-    if (allow_dead_tid && errno == ESRCH) {
-      ALOGE("tid exited before attach completed: tid %d", tid);
-    } else {
-      *detach_failed = true;
-      ALOGE("detach failed: tid %d, %s", tid, strerror(errno));
-    }
-  }
-  return -1;
 }
 
 #define MEMORY_BYTES_TO_DUMP 256
diff --git a/debuggerd/utility.h b/debuggerd/utility.h
index 8bef1924e..ed08ddcbb 100644
--- a/debuggerd/utility.h
+++ b/debuggerd/utility.h
@@ -74,7 +74,7 @@ enum logtype {
 void _LOG(log_t* log, logtype ltype, const char *fmt, ...)
         __attribute__ ((format(printf, 3, 4)));
 
-int wait_for_sigstop(pid_t, int*, bool*);
+int wait_for_signal(pid_t tid, int* total_sleep_time_usec);
 
 void dump_memory(log_t* log, Backtrace* backtrace, uintptr_t addr, const char* fmt, ...);