Fix debuggerd issues.

- Fix a problem where a tid exits before the attach completes, and it
  causes debuggerd to self terminate.
- Fix a problem where sibling tid dumps do not properly wait for the tid
  to get signalled.

Bug: 17800180
Bug: 12567315
Change-Id: Ic3cd619cc2c72402f9a45f14abeed4721b50d64d

Author: cferris1000

debuggerd/backtrace.cpp

@@ -88,7 +88,9 @@ static void dump_thread(
     return;
   }
 
-  wait_for_stop(tid, total_sleep_time_usec);
+  if (!attached && wait_for_sigstop(tid, total_sleep_time_usec, detach_failed) == -1) {
+    return;
+  }
 
   UniquePtr<Backtrace> backtrace(Backtrace::Create(tid, BACKTRACE_CURRENT_THREAD));
   if (backtrace->Unwind(0)) {

debuggerd/debuggerd.cpp

@@ -77,7 +77,7 @@ static void wait_for_user_action(const debugger_request_t &request) {
         "*\n"
         "* Wait for gdb to start, then press the VOLUME DOWN key\n"
         "* to let the process continue crashing.\n"
-        "********************************************************\n",
+        "********************************************************",
         request.pid, exe, request.tid);
 
   // Wait for VOLUME DOWN.
@@ -129,11 +129,11 @@ static int read_request(int fd, debugger_request_t* out_request) {
   socklen_t len = sizeof(cr);
   int status = getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &cr, &len);
   if (status != 0) {
-    ALOGE("cannot get credentials\n");
+    ALOGE("cannot get credentials");
     return -1;
   }
 
-  ALOGV("reading tid\n");
+  ALOGV("reading tid");
   fcntl(fd, F_SETFL, O_NONBLOCK);
 
   pollfd pollfds[1];
@@ -227,6 +227,7 @@ static void handle_request(int fd) {
       ALOGE("ptrace attach failed: %s\n", strerror(errno));
     } else {
       bool detach_failed = false;
+      bool tid_unresponsive = false;
       bool attach_gdb = should_attach_gdb(&request);
       if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) {
         ALOGE("failed responding to client: %s\n", strerror(errno));
@@ -240,8 +241,9 @@ static void handle_request(int fd) {
 
         int total_sleep_time_usec = 0;
         for (;;) {
-          int signal = wait_for_signal(request.tid, &total_sleep_time_usec);
-          if (signal < 0) {
+          int signal = wait_for_sigstop(request.tid, &total_sleep_time_usec, &detach_failed);
+          if (signal == -1) {
+            tid_unresponsive = true;
             break;
           }
 
@@ -308,27 +310,21 @@ static void handle_request(int fd) {
         free(tombstone_path);
       }
 
-      ALOGV("detaching\n");
-      if (attach_gdb) {
-        // stop the process so we can debug
-        kill(request.pid, SIGSTOP);
-
-        // detach so we can attach gdbserver
-        if (ptrace(PTRACE_DETACH, request.tid, 0, 0)) {
-          ALOGE("ptrace detach from %d failed: %s\n", request.tid, strerror(errno));
-          detach_failed = true;
+      if (!tid_unresponsive) {
+        ALOGV("detaching");
+        if (attach_gdb) {
+          // stop the process so we can debug
+          kill(request.pid, SIGSTOP);
         }
-
-        // 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);
-      } else {
-        // just detach
         if (ptrace(PTRACE_DETACH, request.tid, 0, 0)) {
-          ALOGE("ptrace detach from %d failed: %s\n", request.tid, strerror(errno));
+          ALOGE("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);
         }
       }
 

debuggerd/tombstone.cpp

@@ -374,11 +374,7 @@ static void dump_nearby_maps(BacktraceMap* map, log_t* log, pid_t tid) {
   }
 }
 
-static void dump_thread(
-    Backtrace* backtrace, log_t* log, int* total_sleep_time_usec) {
-
-  wait_for_stop(backtrace->Tid(), total_sleep_time_usec);
-
+static void dump_thread(Backtrace* backtrace, log_t* log) {
   dump_registers(log, backtrace->Tid());
   dump_backtrace_and_stack(backtrace, log);
 
@@ -421,13 +417,17 @@ static bool dump_sibling_thread_report(
       continue;
     }
 
+    if (wait_for_sigstop(new_tid, total_sleep_time_usec, &detach_failed) == -1) {
+      continue;
+    }
+
     log->current_tid = new_tid;
     _LOG(log, logtype::THREAD, "--- --- --- --- --- --- --- --- --- --- --- --- --- --- --- ---\n");
     dump_thread_info(log, pid, new_tid);
 
     UniquePtr<Backtrace> backtrace(Backtrace::Create(pid, new_tid, map));
     if (backtrace->Unwind(0)) {
-      dump_thread(backtrace.get(), log, total_sleep_time_usec);
+      dump_thread(backtrace.get(), log);
     }
 
     log->current_tid = log->crashed_tid;
@@ -628,7 +628,7 @@ static bool dump_crash(log_t* log, pid_t pid, pid_t tid, int signal, int si_code
   UniquePtr<Backtrace> backtrace(Backtrace::Create(pid, tid, map.get()));
   if (backtrace->Unwind(0)) {
     dump_abort_message(backtrace.get(), log, abort_msg_address);
-    dump_thread(backtrace.get(), log, total_sleep_time_usec);
+    dump_thread(backtrace.get(), log);
   }
 
   if (want_logs) {

debuggerd/utility.cpp

@@ -28,8 +28,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
+const int SLEEP_TIME_USEC = 50000;         // 0.05 seconds
+const int MAX_TOTAL_SLEEP_USEC = 10000000; // 10 seconds
 
 static int write_to_am(int fd, const char* buf, int len) {
   int to_write = len;
@@ -91,48 +91,44 @@ void _LOG(log_t* log, enum logtype ltype, const char* fmt, ...) {
   }
 }
 
-int wait_for_signal(pid_t tid, int* total_sleep_time_usec) {
+int wait_for_sigstop(pid_t tid, int* total_sleep_time_usec, bool* detach_failed) {
+  bool allow_dead_tid = false;
   for (;;) {
     int status;
-    pid_t n = waitpid(tid, &status, __WALL | WNOHANG);
-    if (n < 0) {
-      if (errno == EAGAIN)
-        continue;
-      ALOGE("waitpid failed: %s\n", strerror(errno));
-      return -1;
-    } else if (n > 0) {
-      ALOGV("waitpid: n=%d status=%08x\n", n, 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;
+    } else if (n == tid) {
       if (WIFSTOPPED(status)) {
         return WSTOPSIG(status);
       } else {
         ALOGE("unexpected waitpid response: n=%d, status=%08x\n", n, status);
-        return -1;
+        // 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;
       }
     }
 
-    if (*total_sleep_time_usec > max_total_sleep_usec) {
-      ALOGE("timed out waiting for tid=%d to die\n", tid);
-      return -1;
+    if (*total_sleep_time_usec > MAX_TOTAL_SLEEP_USEC) {
+      ALOGE("timed out waiting for stop signal: tid=%d", tid);
+      break;
     }
 
-    // not ready yet
-    ALOGV("not ready yet\n");
-    usleep(sleep_time_usec);
-    *total_sleep_time_usec += sleep_time_usec;
+    usleep(SLEEP_TIME_USEC);
+    *total_sleep_time_usec += SLEEP_TIME_USEC;
   }
-}
 
-void wait_for_stop(pid_t tid, int* total_sleep_time_usec) {
-  siginfo_t si;
-  while (TEMP_FAILURE_RETRY(ptrace(PTRACE_GETSIGINFO, tid, 0, &si)) < 0 && errno == ESRCH) {
-    if (*total_sleep_time_usec > max_total_sleep_usec) {
-      ALOGE("timed out waiting for tid=%d to stop\n", tid);
-      break;
+  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));
     }
-
-    usleep(sleep_time_usec);
-    *total_sleep_time_usec += sleep_time_usec;
   }
+  return -1;
 }
 
 #if defined (__mips__)

debuggerd/utility.h

@@ -67,12 +67,11 @@ enum logtype {
   LOGS
 };
 
-/* Log information onto the tombstone. */
+// Log information onto the tombstone.
 void _LOG(log_t* log, logtype ltype, const char *fmt, ...)
         __attribute__ ((format(printf, 3, 4)));
 
-int wait_for_signal(pid_t tid, int* total_sleep_time_usec);
-void wait_for_stop(pid_t tid, int* total_sleep_time_usec);
+int wait_for_sigstop(pid_t, int*, bool*);
 
 void dump_memory(log_t* log, pid_t tid, uintptr_t addr);