tombstoned: don't create tombstones for failed dumps.

Instead of creating tombstone FDs in place and passing them out to
crash_dump directly, create them as O_TMPFILEs and link them into place
when crash_dump reports success, to avoid creating empty tombstones
in cases like an aborting thread racing with another thread that
manages to cleanly exit_group before the dump finishes.

Bug: http://b/77729983
Test: debuggerd_test
Test: adb shell 'for x in `seq 0 50`; do crasher; done'
Change-Id: I31ce4fd4a524abf8bde57152450209483d9d0ba9

1 files changed, 47 insertions, 28 deletions

diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp
index 1bf8f14d0..5dffa5b3c 100644
--- a/debuggerd/tombstoned/tombstoned.cpp
+++ b/debuggerd/tombstoned/tombstoned.cpp
@@ -61,10 +61,10 @@ enum CrashStatus {
 struct Crash {
   ~Crash() { event_free(crash_event); }
 
-  unique_fd crash_fd;
+  unique_fd crash_tombstone_fd;
+  unique_fd crash_socket_fd;
   pid_t crash_pid;
   event* crash_event = nullptr;
-  std::string crash_path;
 
   DebuggerdDumpType crash_type;
 };
@@ -109,24 +109,27 @@ class CrashQueue {
     return &queue;
   }
 
-  std::pair<unique_fd, std::string> get_output() {
-    unique_fd result;
-    std::string file_name = StringPrintf("%s%02d", file_name_prefix_.c_str(), next_artifact_);
-
-    // Unlink and create the file, instead of using O_TRUNC, to avoid two processes
-    // interleaving their output in case we ever get into that situation.
-    if (unlinkat(dir_fd_, file_name.c_str(), 0) != 0 && errno != ENOENT) {
-      PLOG(FATAL) << "failed to unlink tombstone at " << dir_path_ << "/" << file_name;
-    }
-
-    result.reset(openat(dir_fd_, file_name.c_str(),
-                        O_CREAT | O_EXCL | O_WRONLY | O_APPEND | O_CLOEXEC, 0640));
+  unique_fd get_output() {
+    unique_fd result(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640));
     if (result == -1) {
-      PLOG(FATAL) << "failed to create tombstone at " << dir_path_ << "/" << file_name;
+      // We might not have O_TMPFILE. Try creating and unlinking instead.
+      result.reset(
+          openat(dir_fd_, ".temporary", O_WRONLY | O_APPEND | O_CREAT | O_TRUNC | O_CLOEXEC, 0640));
+      if (result == -1) {
+        PLOG(FATAL) << "failed to create temporary tombstone in " << dir_path_;
+      }
+      if (unlinkat(dir_fd_, ".temporary", 0) != 0) {
+        PLOG(FATAL) << "failed to unlink temporary tombstone";
+      }
     }
+    return result;
+  }
 
+  std::string get_next_artifact_path() {
+    std::string file_name =
+        StringPrintf("%s/%s%02d", dir_path_.c_str(), file_name_prefix_.c_str(), next_artifact_);
     next_artifact_ = (next_artifact_ + 1) % max_artifacts_;
-    return {std::move(result), dir_path_ + "/" + file_name};
+    return file_name;
   }
 
   bool maybe_enqueue_crash(Crash* crash) {
@@ -203,14 +206,17 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg);
 
 static void perform_request(Crash* crash) {
   unique_fd output_fd;
-  if (!intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd)) {
-    std::tie(output_fd, crash->crash_path) = CrashQueue::for_crash(crash)->get_output();
+  bool intercepted =
+      intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd);
+  if (!intercepted) {
+    output_fd = CrashQueue::for_crash(crash)->get_output();
+    crash->crash_tombstone_fd.reset(dup(output_fd.get()));
   }
 
   TombstonedCrashPacket response = {
     .packet_type = CrashPacketType::kPerformDump
   };
-  ssize_t rc = send_fd(crash->crash_fd, &response, sizeof(response), std::move(output_fd));
+  ssize_t rc = send_fd(crash->crash_socket_fd, &response, sizeof(response), std::move(output_fd));
   if (rc == -1) {
     PLOG(WARNING) << "failed to send response to CrashRequest";
     goto fail;
@@ -222,7 +228,7 @@ static void perform_request(Crash* crash) {
     struct timeval timeout = { 10, 0 };
 
     event_base* base = event_get_base(crash->crash_event);
-    event_assign(crash->crash_event, base, crash->crash_fd, EV_TIMEOUT | EV_READ,
+    event_assign(crash->crash_event, base, crash->crash_socket_fd, EV_TIMEOUT | EV_READ,
                  crash_completed_cb, crash);
     event_add(crash->crash_event, &timeout);
   }
@@ -243,7 +249,7 @@ static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, so
   // and only native crashes on the native socket.
   struct timeval timeout = { 1, 0 };
   event* crash_event = event_new(base, sockfd, EV_TIMEOUT | EV_READ, crash_request_cb, crash);
-  crash->crash_fd.reset(sockfd);
+  crash->crash_socket_fd.reset(sockfd);
   crash->crash_event = crash_event;
   event_add(crash_event, &timeout);
 }
@@ -342,14 +348,27 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) {
     goto fail;
   }
 
-  if (!crash->crash_path.empty()) {
-    if (crash->crash_type == kDebuggerdJavaBacktrace) {
-      LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << crash->crash_path;
+  if (crash->crash_tombstone_fd != -1) {
+    std::string fd_path = StringPrintf("/proc/self/fd/%d", crash->crash_tombstone_fd.get());
+    std::string tombstone_path = CrashQueue::for_crash(crash)->get_next_artifact_path();
+    int rc = unlink(tombstone_path.c_str());
+    if (rc != 0) {
+      PLOG(ERROR) << "failed to unlink tombstone at " << tombstone_path;
+      goto fail;
+    }
+
+    rc = linkat(AT_FDCWD, fd_path.c_str(), AT_FDCWD, tombstone_path.c_str(), AT_SYMLINK_FOLLOW);
+    if (rc != 0) {
+      PLOG(ERROR) << "failed to link tombstone";
     } else {
-      // NOTE: Several tools parse this log message to figure out where the
-      // tombstone associated with a given native crash was written. Any changes
-      // to this message must be carefully considered.
-      LOG(ERROR) << "Tombstone written to: " << crash->crash_path;
+      if (crash->crash_type == kDebuggerdJavaBacktrace) {
+        LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << tombstone_path;
+      } else {
+        // NOTE: Several tools parse this log message to figure out where the
+        // tombstone associated with a given native crash was written. Any changes
+        // to this message must be carefully considered.
+        LOG(ERROR) << "Tombstone written to: " << tombstone_path;
+      }
     }
   }