Add ability to handle multiple intercepts per pid.

While doing this, refactor the intercept code to be easier to understand.

The primary use case for this is to perform a parallel stack dump (both Java and native) for specific ANRs.

Add tests for all of the different intercept conditions.

Modify the tests to display the error message from the intercept
response if there is an error.

Bug: 254634348
Test: All unit tests pass.
Test: Ran debuggerd on native and java processes.
Test: Created a bugreport without error.
Change-Id: Ic531ccee05b9a470748b815cf109e0076150a0b6

debuggerd/common/include/dump_type.h

@@ -28,26 +28,24 @@ enum DebuggerdDumpType : uint8_t {
   kDebuggerdTombstoneProto,
 };
 
-inline std::ostream& operator<<(std::ostream& stream, const DebuggerdDumpType& rhs) {
+inline const char* get_dump_type_name(const DebuggerdDumpType& dump_type) {
-  switch (rhs) {
+  switch (dump_type) {
     case kDebuggerdNativeBacktrace:
-      stream << "kDebuggerdNativeBacktrace";
+      return "kDebuggerdNativeBacktrace";
-      break;
     case kDebuggerdTombstone:
-      stream << "kDebuggerdTombstone";
+      return "kDebuggerdTombstone";
-      break;
     case kDebuggerdJavaBacktrace:
-      stream << "kDebuggerdJavaBacktrace";
+      return "kDebuggerdJavaBacktrace";
-      break;
     case kDebuggerdAnyIntercept:
-      stream << "kDebuggerdAnyIntercept";
+      return "kDebuggerdAnyIntercept";
-      break;
     case kDebuggerdTombstoneProto:
-      stream << "kDebuggerdTombstoneProto";
+      return "kDebuggerdTombstoneProto";
-      break;
     default:
-      stream << "[unknown]";
+      return "[unknown]";
   }
+}
 
+inline std::ostream& operator<<(std::ostream& stream, const DebuggerdDumpType& rhs) {
+  stream << get_dump_type_name(rhs);
   return stream;
 }

debuggerd/debuggerd_test.cpp

@@ -114,7 +114,7 @@ constexpr char kWaitForDebuggerKey[] = "debug.debuggerd.wait_for_debugger";
                R"(#\d\d pc [0-9a-f]+\s+ \S+ (\(offset 0x[0-9a-f]+\) )?\()" frame_name R"(\+)");
 
 static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, unique_fd* output_fd,
-                                 InterceptStatus* status, DebuggerdDumpType intercept_type) {
+                                 InterceptResponse* response, DebuggerdDumpType intercept_type) {
   intercept_fd->reset(socket_local_client(kTombstonedInterceptSocketName,
                                           ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET));
   if (intercept_fd->get() == -1) {
@@ -155,18 +155,15 @@ static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, uniq
     FAIL() << "failed to send output fd to tombstoned: " << strerror(errno);
   }
 
-  InterceptResponse response;
+  rc = TEMP_FAILURE_RETRY(read(intercept_fd->get(), response, sizeof(*response)));
-  rc = TEMP_FAILURE_RETRY(read(intercept_fd->get(), &response, sizeof(response)));
   if (rc == -1) {
     FAIL() << "failed to read response from tombstoned: " << strerror(errno);
   } else if (rc == 0) {
     FAIL() << "failed to read response from tombstoned (EOF)";
-  } else if (rc != sizeof(response)) {
+  } else if (rc != sizeof(*response)) {
-    FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(response)
+    FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(*response)
            << ", received " << rc;
   }
-
-  *status = response.status;
 }
 
 static bool pac_supported() {
@@ -225,9 +222,10 @@ void CrasherTest::StartIntercept(unique_fd* output_fd, DebuggerdDumpType interce
     FAIL() << "crasher hasn't been started";
   }
 
-  InterceptStatus status;
+  InterceptResponse response = {};
-  tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd, &status, intercept_type);
+  tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd, &response, intercept_type);
-  ASSERT_EQ(InterceptStatus::kRegistered, status);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
 }
 
 void CrasherTest::FinishIntercept(int* result) {
@@ -1744,9 +1742,10 @@ TEST(tombstoned, no_notify) {
     pid_t pid = 123'456'789 + i;
 
     unique_fd intercept_fd, output_fd;
-    InterceptStatus status;
+    InterceptResponse response = {};
-    tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone);
+    tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone);
-    ASSERT_EQ(InterceptStatus::kRegistered, status);
+    ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+        << "Error message: " << response.error_message;
 
     {
       unique_fd tombstoned_socket, input_fd;
@@ -1778,9 +1777,10 @@ TEST(tombstoned, stress) {
       pid_t pid = pid_base + dump;
 
       unique_fd intercept_fd, output_fd;
-      InterceptStatus status;
+      InterceptResponse response = {};
-      tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone);
+      tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone);
-      ASSERT_EQ(InterceptStatus::kRegistered, status);
+      ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+          << "Error messeage: " << response.error_message;
 
       // Pretend to crash, and then immediately close the socket.
       unique_fd sockfd(socket_local_client(kTombstonedCrashSocketName,
@@ -1811,9 +1811,10 @@ TEST(tombstoned, stress) {
       pid_t pid = pid_base + dump;
 
       unique_fd intercept_fd, output_fd;
-      InterceptStatus status;
+      InterceptResponse response = {};
-      tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone);
+      tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone);
-      ASSERT_EQ(InterceptStatus::kRegistered, status);
+      ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+          << "Error message: " << response.error_message;
 
       {
         unique_fd tombstoned_socket, input_fd;
@@ -1838,16 +1839,17 @@ TEST(tombstoned, stress) {
   }
 }
 
-TEST(tombstoned, java_trace_intercept_smoke) {
+TEST(tombstoned, intercept_java_trace_smoke) {
   // Using a "real" PID is a little dangerous here - if the test fails
   // or crashes, we might end up getting a bogus / unreliable stack
   // trace.
   const pid_t self = getpid();
 
   unique_fd intercept_fd, output_fd;
-  InterceptStatus status;
+  InterceptResponse response = {};
-  tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace);
+  tombstoned_intercept(self, &intercept_fd, &output_fd, &response, kDebuggerdJavaBacktrace);
-  ASSERT_EQ(InterceptStatus::kRegistered, status);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
 
   // First connect to tombstoned requesting a native tombstone. This
   // should result in a "regular" FD and not the installed intercept.
@@ -1869,25 +1871,96 @@ TEST(tombstoned, java_trace_intercept_smoke) {
   ASSERT_STREQ("java", outbuf);
 }
 
-TEST(tombstoned, multiple_intercepts) {
+TEST(tombstoned, intercept_multiple_dump_types) {
   const pid_t fake_pid = 1'234'567;
   unique_fd intercept_fd, output_fd;
-  InterceptStatus status;
+  InterceptResponse response = {};
-  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace);
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdJavaBacktrace);
-  ASSERT_EQ(InterceptStatus::kRegistered, status);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
 
   unique_fd intercept_fd_2, output_fd_2;
-  tombstoned_intercept(fake_pid, &intercept_fd_2, &output_fd_2, &status, kDebuggerdNativeBacktrace);
+  tombstoned_intercept(fake_pid, &intercept_fd_2, &output_fd_2, &response,
-  ASSERT_EQ(InterceptStatus::kFailedAlreadyRegistered, status);
+                       kDebuggerdNativeBacktrace);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+}
+
+TEST(tombstoned, intercept_bad_pid) {
+  const pid_t fake_pid = -1;
+  unique_fd intercept_fd, output_fd;
+  InterceptResponse response = {};
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdNativeBacktrace);
+  ASSERT_EQ(InterceptStatus::kFailed, response.status)
+      << "Error message: " << response.error_message;
+  ASSERT_MATCH(response.error_message, "bad pid");
+}
+
+TEST(tombstoned, intercept_bad_dump_types) {
+  const pid_t fake_pid = 1'234'567;
+  unique_fd intercept_fd, output_fd;
+  InterceptResponse response = {};
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response,
+                       static_cast<DebuggerdDumpType>(20));
+  ASSERT_EQ(InterceptStatus::kFailed, response.status)
+      << "Error message: " << response.error_message;
+  ASSERT_MATCH(response.error_message, "bad dump type \\[unknown\\]");
+
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdAnyIntercept);
+  ASSERT_EQ(InterceptStatus::kFailed, response.status)
+      << "Error message: " << response.error_message;
+  ASSERT_MATCH(response.error_message, "bad dump type kDebuggerdAnyIntercept");
+
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstoneProto);
+  ASSERT_EQ(InterceptStatus::kFailed, response.status)
+      << "Error message: " << response.error_message;
+  ASSERT_MATCH(response.error_message, "bad dump type kDebuggerdTombstoneProto");
+}
+
+TEST(tombstoned, intercept_already_registered) {
+  const pid_t fake_pid = 1'234'567;
+  unique_fd intercept_fd1, output_fd1;
+  InterceptResponse response = {};
+  tombstoned_intercept(fake_pid, &intercept_fd1, &output_fd1, &response, kDebuggerdTombstone);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  unique_fd intercept_fd2, output_fd2;
+  tombstoned_intercept(fake_pid, &intercept_fd2, &output_fd2, &response, kDebuggerdTombstone);
+  ASSERT_EQ(InterceptStatus::kFailedAlreadyRegistered, response.status)
+      << "Error message: " << response.error_message;
+  ASSERT_MATCH(response.error_message, "already registered, type kDebuggerdTombstone");
+}
+
+TEST(tombstoned, intercept_tombstone_proto_matched_to_tombstone) {
+  const pid_t fake_pid = 1'234'567;
+
+  unique_fd intercept_fd, output_fd;
+  InterceptResponse response = {};
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  const char data[] = "tombstone_proto";
+  unique_fd tombstoned_socket, input_fd;
+  ASSERT_TRUE(
+      tombstoned_connect(fake_pid, &tombstoned_socket, &input_fd, kDebuggerdTombstoneProto));
+  ASSERT_TRUE(android::base::WriteFully(input_fd.get(), data, sizeof(data)));
+  tombstoned_notify_completion(tombstoned_socket.get());
+
+  char outbuf[sizeof(data)];
+  ASSERT_TRUE(android::base::ReadFully(output_fd.get(), outbuf, sizeof(outbuf)));
+  ASSERT_STREQ("tombstone_proto", outbuf);
 }
 
 TEST(tombstoned, intercept_any) {
   const pid_t fake_pid = 1'234'567;
 
   unique_fd intercept_fd, output_fd;
-  InterceptStatus status;
+  InterceptResponse response = {};
-  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &status, kDebuggerdNativeBacktrace);
+  tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdNativeBacktrace);
-  ASSERT_EQ(InterceptStatus::kRegistered, status);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
 
   const char any[] = "any";
   unique_fd tombstoned_socket, input_fd;
@@ -1900,6 +1973,77 @@ TEST(tombstoned, intercept_any) {
   ASSERT_STREQ("any", outbuf);
 }
 
+TEST(tombstoned, intercept_any_failed_with_multiple_intercepts) {
+  const pid_t fake_pid = 1'234'567;
+
+  InterceptResponse response = {};
+  unique_fd intercept_fd1, output_fd1;
+  tombstoned_intercept(fake_pid, &intercept_fd1, &output_fd1, &response, kDebuggerdNativeBacktrace);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  unique_fd intercept_fd2, output_fd2;
+  tombstoned_intercept(fake_pid, &intercept_fd2, &output_fd2, &response, kDebuggerdJavaBacktrace);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  unique_fd tombstoned_socket, input_fd;
+  ASSERT_FALSE(tombstoned_connect(fake_pid, &tombstoned_socket, &input_fd, kDebuggerdAnyIntercept));
+}
+
+TEST(tombstoned, intercept_multiple_verify_intercept) {
+  // Need to use our pid for java since that will verify the pid.
+  const pid_t fake_pid = getpid();
+
+  InterceptResponse response = {};
+  unique_fd intercept_fd1, output_fd1;
+  tombstoned_intercept(fake_pid, &intercept_fd1, &output_fd1, &response, kDebuggerdNativeBacktrace);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  unique_fd intercept_fd2, output_fd2;
+  tombstoned_intercept(fake_pid, &intercept_fd2, &output_fd2, &response, kDebuggerdJavaBacktrace);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  unique_fd intercept_fd3, output_fd3;
+  tombstoned_intercept(fake_pid, &intercept_fd3, &output_fd3, &response, kDebuggerdTombstone);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
+
+  const char native_data[] = "native";
+  unique_fd tombstoned_socket1, input_fd1;
+  ASSERT_TRUE(
+      tombstoned_connect(fake_pid, &tombstoned_socket1, &input_fd1, kDebuggerdNativeBacktrace));
+  ASSERT_TRUE(android::base::WriteFully(input_fd1.get(), native_data, sizeof(native_data)));
+  tombstoned_notify_completion(tombstoned_socket1.get());
+
+  char native_outbuf[sizeof(native_data)];
+  ASSERT_TRUE(android::base::ReadFully(output_fd1.get(), native_outbuf, sizeof(native_outbuf)));
+  ASSERT_STREQ("native", native_outbuf);
+
+  const char java_data[] = "java";
+  unique_fd tombstoned_socket2, input_fd2;
+  ASSERT_TRUE(
+      tombstoned_connect(fake_pid, &tombstoned_socket2, &input_fd2, kDebuggerdJavaBacktrace));
+  ASSERT_TRUE(android::base::WriteFully(input_fd2.get(), java_data, sizeof(java_data)));
+  tombstoned_notify_completion(tombstoned_socket2.get());
+
+  char java_outbuf[sizeof(java_data)];
+  ASSERT_TRUE(android::base::ReadFully(output_fd2.get(), java_outbuf, sizeof(java_outbuf)));
+  ASSERT_STREQ("java", java_outbuf);
+
+  const char tomb_data[] = "tombstone";
+  unique_fd tombstoned_socket3, input_fd3;
+  ASSERT_TRUE(tombstoned_connect(fake_pid, &tombstoned_socket3, &input_fd3, kDebuggerdTombstone));
+  ASSERT_TRUE(android::base::WriteFully(input_fd3.get(), tomb_data, sizeof(tomb_data)));
+  tombstoned_notify_completion(tombstoned_socket3.get());
+
+  char tomb_outbuf[sizeof(tomb_data)];
+  ASSERT_TRUE(android::base::ReadFully(output_fd3.get(), tomb_outbuf, sizeof(tomb_outbuf)));
+  ASSERT_STREQ("tombstone", tomb_outbuf);
+}
+
 TEST(tombstoned, interceptless_backtrace) {
   // Generate 50 backtraces, and then check to see that we haven't created 50 new tombstones.
   auto get_tombstone_timestamps = []() -> std::map<int, time_t> {
@@ -2132,10 +2276,11 @@ TEST(tombstoned, proto) {
 TEST(tombstoned, proto_intercept) {
   const pid_t self = getpid();
   unique_fd intercept_fd, output_fd;
-  InterceptStatus status;
 
-  tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdTombstone);
+  InterceptResponse response = {};
-  ASSERT_EQ(InterceptStatus::kRegistered, status);
+  tombstoned_intercept(self, &intercept_fd, &output_fd, &response, kDebuggerdTombstone);
+  ASSERT_EQ(InterceptStatus::kRegistered, response.status)
+      << "Error message: " << response.error_message;
 
   unique_fd tombstoned_socket, text_fd, proto_fd;
   ASSERT_TRUE(

debuggerd/protocol.h

@@ -65,7 +65,7 @@ struct InterceptRequest {
 };
 
 enum class InterceptStatus : uint8_t {
-  // Returned when an intercept of a different type has already been
+  // Returned when an intercept of the same type has already been
   // registered (and is active) for a given PID.
   kFailedAlreadyRegistered,
   // Returned in all other failure cases.

debuggerd/tombstoned/intercept_manager.cpp

@@ -19,7 +19,10 @@
 #include <inttypes.h>
 #include <sys/types.h>
 
+#include <limits>
+#include <memory>
 #include <unordered_map>
+#include <utility>
 
 #include <event2/event.h>
 #include <event2/listener.h>
@@ -36,8 +39,7 @@ using android::base::ReceiveFileDescriptors;
 using android::base::unique_fd;
 
 static void intercept_close_cb(evutil_socket_t sockfd, short event, void* arg) {
-  auto intercept = reinterpret_cast<Intercept*>(arg);
+  std::unique_ptr<Intercept> intercept(reinterpret_cast<Intercept*>(arg));
-  InterceptManager* intercept_manager = intercept->intercept_manager;
 
   CHECK_EQ(sockfd, intercept->sockfd.get());
 
@@ -46,131 +48,108 @@ static void intercept_close_cb(evutil_socket_t sockfd, short event, void* arg) {
 
   // Ownership of intercept differs based on whether we've registered it with InterceptManager.
   if (!intercept->registered) {
-    delete intercept;
+    LOG(WARNING) << "intercept for pid " << intercept->pid << " and type " << intercept->dump_type
-  } else {
+                 << " closed before being registered.";
-    auto it = intercept_manager->intercepts.find(intercept->intercept_pid);
+    return;
-    if (it == intercept_manager->intercepts.end()) {
-      LOG(FATAL) << "intercept close callback called after intercept was already removed?";
-    }
-    if (it->second.get() != intercept) {
-      LOG(FATAL) << "intercept close callback has different Intercept from InterceptManager?";
-    }
-
-    const char* reason;
-    if ((event & EV_TIMEOUT) != 0) {
-      reason = "due to timeout";
-    } else {
-      reason = "due to input";
-    }
-
-    LOG(INFO) << "intercept for pid " << intercept->intercept_pid << " and type "
-              << intercept->dump_type << " terminated: " << reason;
-    intercept_manager->intercepts.erase(it);
   }
+
+  const char* reason = (event & EV_TIMEOUT) ? "due to timeout" : "due to input";
+  LOG(INFO) << "intercept for pid " << intercept->pid << " and type " << intercept->dump_type
+            << " terminated: " << reason;
 }
 
-static bool is_intercept_request_valid(const InterceptRequest& request) {
+void InterceptManager::Unregister(Intercept* intercept) {
-  if (request.pid <= 0 || request.pid > std::numeric_limits<pid_t>::max()) {
+  CHECK(intercept->registered);
-    return false;
+  auto pid_entry = intercepts.find(intercept->pid);
+  if (pid_entry == intercepts.end()) {
+    LOG(FATAL) << "No intercepts found for pid " << intercept->pid;
+  }
+  auto& dump_type_hash = pid_entry->second;
+  auto dump_type_entry = dump_type_hash.find(intercept->dump_type);
+  if (dump_type_entry == dump_type_hash.end()) {
+    LOG(FATAL) << "Unknown intercept " << intercept->pid << " " << intercept->dump_type;
+  }
+  if (intercept != dump_type_entry->second) {
+    LOG(FATAL) << "Mismatch pointer trying to unregister intercept " << intercept->pid << " "
+               << intercept->dump_type;
   }
 
-  if (request.dump_type < 0 || request.dump_type > kDebuggerdJavaBacktrace) {
+  dump_type_hash.erase(dump_type_entry);
-    return false;
+  if (dump_type_hash.empty()) {
+    intercepts.erase(pid_entry);
   }
-
-  return true;
 }
 
 static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) {
-  auto intercept = reinterpret_cast<Intercept*>(arg);
+  std::unique_ptr<Intercept> intercept(reinterpret_cast<Intercept*>(arg));
   InterceptManager* intercept_manager = intercept->intercept_manager;
 
   CHECK_EQ(sockfd, intercept->sockfd.get());
 
   if ((ev & EV_TIMEOUT) != 0) {
     LOG(WARNING) << "tombstoned didn't receive InterceptRequest before timeout";
-    goto fail;
+    return;
   } else if ((ev & EV_READ) == 0) {
     LOG(WARNING) << "tombstoned received unexpected event on intercept socket";
-    goto fail;
+    return;
   }
 
-  {
+  unique_fd rcv_fd;
-    unique_fd rcv_fd;
+  InterceptRequest intercept_request;
-    InterceptRequest intercept_request;
+  ssize_t result =
-    ssize_t result =
+      ReceiveFileDescriptors(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd);
-        ReceiveFileDescriptors(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd);
 
-    if (result == -1) {
+  if (result == -1) {
-      PLOG(WARNING) << "failed to read from intercept socket";
+    PLOG(WARNING) << "failed to read from intercept socket";
-      goto fail;
+    return;
-    } else if (result != sizeof(intercept_request)) {
+  }
-      LOG(WARNING) << "intercept socket received short read of length " << result << " (expected "
+  if (result != sizeof(intercept_request)) {
-                   << sizeof(intercept_request) << ")";
+    LOG(WARNING) << "intercept socket received short read of length " << result << " (expected "
-      goto fail;
+                 << sizeof(intercept_request) << ")";
-    }
+    return;
-
-    // Move the received FD to the upper half, in order to more easily notice FD leaks.
-    int moved_fd = fcntl(rcv_fd.get(), F_DUPFD, 512);
-    if (moved_fd == -1) {
-      LOG(WARNING) << "failed to move received fd (" << rcv_fd.get() << ")";
-      goto fail;
-    }
-    rcv_fd.reset(moved_fd);
-
-    // We trust the other side, so only do minimal validity checking.
-    if (!is_intercept_request_valid(intercept_request)) {
-      InterceptResponse response = {};
-      response.status = InterceptStatus::kFailed;
-      snprintf(response.error_message, sizeof(response.error_message), "invalid intercept request");
-      TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response)));
-      goto fail;
-    }
-
-    intercept->intercept_pid = intercept_request.pid;
-    intercept->dump_type = intercept_request.dump_type;
-
-    // Check if it's already registered.
-    if (intercept_manager->intercepts.count(intercept_request.pid) > 0) {
-      InterceptResponse response = {};
-      response.status = InterceptStatus::kFailedAlreadyRegistered;
-      snprintf(response.error_message, sizeof(response.error_message),
-               "pid %" PRId32 " already intercepted, type %d", intercept_request.pid,
-               intercept_request.dump_type);
-      TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response)));
-      LOG(WARNING) << response.error_message;
-      goto fail;
-    }
-
-    // Let the other side know that the intercept has been registered, now that we know we can't
-    // fail. tombstoned is single threaded, so this isn't racy.
-    InterceptResponse response = {};
-    response.status = InterceptStatus::kRegistered;
-    if (TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response))) == -1) {
-      PLOG(WARNING) << "failed to notify interceptor of registration";
-      goto fail;
-    }
-
-    intercept->output_fd = std::move(rcv_fd);
-    intercept_manager->intercepts[intercept_request.pid] = std::unique_ptr<Intercept>(intercept);
-    intercept->registered = true;
-
-    LOG(INFO) << "registered intercept for pid " << intercept_request.pid << " and type "
-              << intercept_request.dump_type;
-
-    // Register a different read event on the socket so that we can remove intercepts if the socket
-    // closes (e.g. if a user CTRL-C's the process that requested the intercept).
-    event_assign(intercept->intercept_event, intercept_manager->base, sockfd, EV_READ | EV_TIMEOUT,
-                 intercept_close_cb, arg);
-
-    struct timeval timeout = {.tv_sec = 10 * android::base::HwTimeoutMultiplier(), .tv_usec = 0};
-    event_add(intercept->intercept_event, &timeout);
   }
 
-  return;
+  // Move the received FD to the upper half, in order to more easily notice FD leaks.
+  int moved_fd = fcntl(rcv_fd.get(), F_DUPFD, 512);
+  if (moved_fd == -1) {
+    LOG(WARNING) << "failed to move received fd (" << rcv_fd.get() << ")";
+    return;
+  }
+  rcv_fd.reset(moved_fd);
 
-fail:
+  // See if we can properly register the intercept.
-  delete intercept;
+  InterceptResponse response = {};
+  if (!intercept_manager->CanRegister(intercept_request, response)) {
+    TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response)));
+    LOG(WARNING) << response.error_message;
+    return;
+  }
+
+  // Let the other side know that the intercept has been registered, now that we know we can't
+  // fail. tombstoned is single threaded, so this isn't racy.
+  response.status = InterceptStatus::kRegistered;
+  if (TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response))) == -1) {
+    PLOG(WARNING) << "failed to notify interceptor of registration";
+    return;
+  }
+
+  intercept->pid = intercept_request.pid;
+  intercept->dump_type = intercept_request.dump_type;
+  intercept->output_fd = std::move(rcv_fd);
+  intercept_manager->Register(intercept.get());
+
+  LOG(INFO) << "registered intercept for pid " << intercept_request.pid << " and type "
+            << intercept_request.dump_type;
+
+  // Register a different read event on the socket so that we can remove intercepts if the socket
+  // closes (e.g. if a user CTRL-C's the process that requested the intercept).
+  event_assign(intercept->intercept_event, intercept_manager->base, sockfd, EV_READ | EV_TIMEOUT,
+               intercept_close_cb, arg);
+
+  // If no request comes in, then this will close the intercept and free the pointer.
+  struct timeval timeout = {.tv_sec = 10 * android::base::HwTimeoutMultiplier(), .tv_usec = 0};
+  event_add(intercept->intercept_event, &timeout);
+  intercept.release();
 }
 
 static void intercept_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int,
@@ -187,41 +166,97 @@ static void intercept_accept_cb(evconnlistener* listener, evutil_socket_t sockfd
   event_add(intercept_event, &timeout);
 }
 
+Intercept::~Intercept() {
+  event_free(intercept_event);
+  if (registered) {
+    CHECK(intercept_manager != nullptr);
+    intercept_manager->Unregister(this);
+  }
+}
+
 InterceptManager::InterceptManager(event_base* base, int intercept_socket) : base(base) {
   this->listener = evconnlistener_new(base, intercept_accept_cb, this, LEV_OPT_CLOSE_ON_FREE,
                                       /* backlog */ -1, intercept_socket);
 }
 
-bool dump_types_compatible(DebuggerdDumpType interceptor, DebuggerdDumpType dump) {
+static DebuggerdDumpType canonical_dump_type(const DebuggerdDumpType dump_type) {
-  if (interceptor == dump) {
+  // kDebuggerdTombstone and kDebuggerdTombstoneProto should be treated as
-    return true;
+  // a single dump_type for intercepts (kDebuggerdTombstone).
+  if (dump_type == kDebuggerdTombstoneProto) {
+    return kDebuggerdTombstone;
   }
-
+  return dump_type;
-  if (interceptor == kDebuggerdTombstone && dump == kDebuggerdTombstoneProto) {
-    return true;
-  }
-
-  return false;
 }
 
-bool InterceptManager::GetIntercept(pid_t pid, DebuggerdDumpType dump_type,
+Intercept* InterceptManager::Get(const pid_t pid, const DebuggerdDumpType dump_type) {
-                                    android::base::unique_fd* out_fd) {
+  auto pid_entry = intercepts.find(pid);
-  auto it = this->intercepts.find(pid);
+  if (pid_entry == intercepts.end()) {
-  if (it == this->intercepts.end()) {
+    return nullptr;
+  }
+
+  const auto& dump_type_hash = pid_entry->second;
+  auto dump_type_entry = dump_type_hash.find(canonical_dump_type(dump_type));
+  if (dump_type_entry == dump_type_hash.end()) {
+    if (dump_type != kDebuggerdAnyIntercept) {
+      return nullptr;
+    }
+    // If doing a dump with an any intercept, only allow an any to match
+    // a single intercept. If there are multiple dump types with intercepts
+    // then there would be no way to figure out which to use.
+    if (dump_type_hash.size() != 1) {
+      LOG(WARNING) << "Cannot intercept using kDebuggerdAnyIntercept: there is more than one "
+                      "intercept registered for pid "
+                   << pid;
+      return nullptr;
+    }
+    dump_type_entry = dump_type_hash.begin();
+  }
+  return dump_type_entry->second;
+}
+
+bool InterceptManager::CanRegister(const InterceptRequest& request, InterceptResponse& response) {
+  if (request.pid <= 0 || request.pid > std::numeric_limits<pid_t>::max()) {
+    response.status = InterceptStatus::kFailed;
+    snprintf(response.error_message, sizeof(response.error_message),
+             "invalid intercept request: bad pid %" PRId32, request.pid);
+    return false;
+  }
+  if (request.dump_type < 0 || request.dump_type > kDebuggerdJavaBacktrace) {
+    response.status = InterceptStatus::kFailed;
+    snprintf(response.error_message, sizeof(response.error_message),
+             "invalid intercept request: bad dump type %s", get_dump_type_name(request.dump_type));
     return false;
   }
 
-  if (dump_type == kDebuggerdAnyIntercept) {
+  if (Get(request.pid, request.dump_type) != nullptr) {
-    LOG(INFO) << "found registered intercept of type " << it->second->dump_type
+    response.status = InterceptStatus::kFailedAlreadyRegistered;
-              << " for requested type kDebuggerdAnyIntercept";
+    snprintf(response.error_message, sizeof(response.error_message),
-  } else if (!dump_types_compatible(it->second->dump_type, dump_type)) {
+             "pid %" PRId32 " already registered, type %s", request.pid,
-    LOG(WARNING) << "found non-matching intercept of type " << it->second->dump_type
+             get_dump_type_name(request.dump_type));
-                 << " for requested type: " << dump_type;
     return false;
   }
 
-  auto intercept = std::move(it->second);
+  return true;
-  this->intercepts.erase(it);
+}
+
+void InterceptManager::Register(Intercept* intercept) {
+  CHECK(!intercept->registered);
+  auto& dump_type_hash = intercepts[intercept->pid];
+  dump_type_hash[canonical_dump_type(intercept->dump_type)] = intercept;
+  intercept->registered = true;
+}
+
+bool InterceptManager::FindIntercept(pid_t pid, DebuggerdDumpType dump_type,
+                                     android::base::unique_fd* out_fd) {
+  Intercept* intercept = Get(pid, dump_type);
+  if (intercept == nullptr) {
+    return false;
+  }
+
+  if (dump_type != intercept->dump_type) {
+    LOG(INFO) << "found registered intercept of type " << intercept->dump_type
+              << " for requested type " << dump_type;
+  }
 
   LOG(INFO) << "found intercept fd " << intercept->output_fd.get() << " for pid " << pid
             << " and type " << intercept->dump_type;
@@ -230,5 +265,8 @@ bool InterceptManager::GetIntercept(pid_t pid, DebuggerdDumpType dump_type,
   TEMP_FAILURE_RETRY(write(intercept->sockfd, &response, sizeof(response)));
   *out_fd = std::move(intercept->output_fd);
 
+  // Delete the intercept data, which will unregister the intercept and remove the timeout event.
+  delete intercept;
+
   return true;
 }

debuggerd/tombstoned/intercept_manager.h

@@ -28,17 +28,17 @@
 #include "dump_type.h"
 
 struct InterceptManager;
+struct InterceptRequest;
+struct InterceptResponse;
 
 struct Intercept {
-  ~Intercept() {
+  ~Intercept();
-    event_free(intercept_event);
-  }
 
   InterceptManager* intercept_manager = nullptr;
   event* intercept_event = nullptr;
   android::base::unique_fd sockfd;
 
-  pid_t intercept_pid = -1;
+  pid_t pid = -1;
   android::base::unique_fd output_fd;
   bool registered = false;
   DebuggerdDumpType dump_type = kDebuggerdNativeBacktrace;
@@ -46,12 +46,17 @@ struct Intercept {
 
 struct InterceptManager {
   event_base* base;
-  std::unordered_map<pid_t, std::unique_ptr<Intercept>> intercepts;
+  std::unordered_map<pid_t, std::unordered_map<DebuggerdDumpType, Intercept*>> intercepts;
   evconnlistener* listener = nullptr;
 
   InterceptManager(event_base* _Nonnull base, int intercept_socket);
   InterceptManager(InterceptManager& copy) = delete;
   InterceptManager(InterceptManager&& move) = delete;
 
-  bool GetIntercept(pid_t pid, DebuggerdDumpType dump_type, android::base::unique_fd* out_fd);
+  bool CanRegister(const InterceptRequest& request, InterceptResponse& response);
+  Intercept* Get(const pid_t pid, const DebuggerdDumpType dump_type);
+  void Register(Intercept* intercept);
+  void Unregister(Intercept* intercept);
+
+  bool FindIntercept(pid_t pid, DebuggerdDumpType dump_type, android::base::unique_fd* out_fd);
 };

debuggerd/tombstoned/tombstoned.cpp

@@ -283,9 +283,7 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg);
 
 static void perform_request(std::unique_ptr<Crash> crash) {
   unique_fd output_fd;
-  bool intercepted =
+  if (intercept_manager->FindIntercept(crash->crash_pid, crash->crash_type, &output_fd)) {
-      intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd);
-  if (intercepted) {
     if (crash->crash_type == kDebuggerdTombstoneProto) {
       crash->output.proto = CrashArtifact::devnull();
     }