tombstoned: switch from goto to RAII. am: 9a61f685d8 am: 42c22410fc

Original change: https://android-review.googlesource.com/c/platform/system/core/+/1515700

MUST ONLY BE SUBMITTED BY AUTOMERGER

Change-Id: Ife8b285c5cb57b3e2bdd19241aa4f475be52a033

debuggerd/tombstoned/tombstoned.cpp

@@ -48,6 +48,8 @@
 using android::base::GetIntProperty;
 using android::base::SendFileDescriptors;
 using android::base::StringPrintf;
+
+using android::base::borrowed_fd;
 using android::base::unique_fd;
 
 static InterceptManager* intercept_manager;
@@ -98,6 +100,10 @@ class CrashQueue {
     return (crash->crash_type == kDebuggerdJavaBacktrace) ? for_anrs() : for_tombstones();
   }
 
+  static CrashQueue* for_crash(const std::unique_ptr<Crash>& crash) {
+    return for_crash(crash.get());
+  }
+
   static CrashQueue* for_tombstones() {
     static CrashQueue queue("/data/tombstones", "tombstone_" /* file_name_prefix */,
                             GetIntProperty("tombstoned.max_tombstone_count", 32),
@@ -137,20 +143,21 @@ class CrashQueue {
     return file_name;
   }
 
-  bool maybe_enqueue_crash(Crash* crash) {
+  // Consumes crash if it returns true, otherwise leaves it untouched.
+  bool maybe_enqueue_crash(std::unique_ptr<Crash>&& crash) {
     if (num_concurrent_dumps_ == max_concurrent_dumps_) {
-      queued_requests_.push_back(crash);
+      queued_requests_.emplace_back(std::move(crash));
       return true;
     }
 
     return false;
   }
 
-  void maybe_dequeue_crashes(void (*handler)(Crash* crash)) {
+  void maybe_dequeue_crashes(void (*handler)(std::unique_ptr<Crash> crash)) {
     while (!queued_requests_.empty() && num_concurrent_dumps_ < max_concurrent_dumps_) {
-      Crash* next_crash = queued_requests_.front();
+      std::unique_ptr<Crash> next_crash = std::move(queued_requests_.front());
       queued_requests_.pop_front();
-      handler(next_crash);
+      handler(std::move(next_crash));
     }
   }
 
@@ -196,7 +203,7 @@ class CrashQueue {
   const size_t max_concurrent_dumps_;
   size_t num_concurrent_dumps_;
 
-  std::deque<Crash*> queued_requests_;
+  std::deque<std::unique_ptr<Crash>> queued_requests_;
 
   DISALLOW_COPY_AND_ASSIGN(CrashQueue);
 };
@@ -209,7 +216,7 @@ static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, so
 static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg);
 static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg);
 
-static void perform_request(Crash* crash) {
+static void perform_request(std::unique_ptr<Crash> crash) {
   unique_fd output_fd;
   bool intercepted =
       intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd);
@@ -232,25 +239,24 @@ static void perform_request(Crash* crash) {
 
   if (rc == -1) {
     PLOG(WARNING) << "failed to send response to CrashRequest";
-    goto fail;
+    return;
   } else if (rc != sizeof(response)) {
     PLOG(WARNING) << "crash socket write returned short";
-    goto fail;
+    return;
-  } else {
-    // TODO: Make this configurable by the interceptor?
-    struct timeval timeout = { 10, 0 };
-
-    event_base* base = event_get_base(crash->crash_event);
-    event_assign(crash->crash_event, base, crash->crash_socket_fd, EV_TIMEOUT | EV_READ,
-                 crash_completed_cb, crash);
-    event_add(crash->crash_event, &timeout);
   }
 
-  CrashQueue::for_crash(crash)->on_crash_started();
+  // TODO: Make this configurable by the interceptor?
-  return;
+  struct timeval timeout = {10, 0};
 
-fail:
+  event_base* base = event_get_base(crash->crash_event);
-  delete crash;
+
+  event_assign(crash->crash_event, base, crash->crash_socket_fd, EV_TIMEOUT | EV_READ,
+               crash_completed_cb, crash.get());
+  event_add(crash->crash_event, &timeout);
+  CrashQueue::for_crash(crash)->on_crash_started();
+
+  // The crash is now owned by the event loop.
+  crash.release();
 }
 
 static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int,
@@ -268,39 +274,37 @@ static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, so
 }
 
 static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) {
-  ssize_t rc;
+  std::unique_ptr<Crash> crash(static_cast<Crash*>(arg));
-  Crash* crash = static_cast<Crash*>(arg);
-
   TombstonedCrashPacket request = {};
 
   if ((ev & EV_TIMEOUT) != 0) {
     LOG(WARNING) << "crash request timed out";
-    goto fail;
+    return;
   } else if ((ev & EV_READ) == 0) {
     LOG(WARNING) << "tombstoned received unexpected event from crash socket";
-    goto fail;
+    return;
   }
 
-  rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request)));
+  ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request)));
   if (rc == -1) {
     PLOG(WARNING) << "failed to read from crash socket";
-    goto fail;
+    return;
   } else if (rc != sizeof(request)) {
     LOG(WARNING) << "crash socket received short read of length " << rc << " (expected "
                  << sizeof(request) << ")";
-    goto fail;
+    return;
   }
 
   if (request.packet_type != CrashPacketType::kDumpRequest) {
     LOG(WARNING) << "unexpected crash packet type, expected kDumpRequest, received  "
                  << StringPrintf("%#2hhX", request.packet_type);
-    goto fail;
+    return;
   }
 
   crash->crash_type = request.packet.dump_request.dump_type;
   if (crash->crash_type < 0 || crash->crash_type > kDebuggerdAnyIntercept) {
     LOG(WARNING) << "unexpected crash dump type: " << crash->crash_type;
-    goto fail;
+    return;
   }
 
   if (crash->crash_type != kDebuggerdJavaBacktrace) {
@@ -314,51 +318,39 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) {
     int ret = getsockopt(sockfd, SOL_SOCKET, SO_PEERCRED, &cr, &len);
     if (ret != 0) {
       PLOG(ERROR) << "Failed to getsockopt(..SO_PEERCRED)";
-      goto fail;
+      return;
     }
 
     crash->crash_pid = cr.pid;
   }
 
-  LOG(INFO) << "received crash request for pid " << crash->crash_pid;
+  pid_t crash_pid = crash->crash_pid;
+  LOG(INFO) << "received crash request for pid " << crash_pid;
 
-  if (CrashQueue::for_crash(crash)->maybe_enqueue_crash(crash)) {
+  if (CrashQueue::for_crash(crash)->maybe_enqueue_crash(std::move(crash))) {
-    LOG(INFO) << "enqueueing crash request for pid " << crash->crash_pid;
+    LOG(INFO) << "enqueueing crash request for pid " << crash_pid;
   } else {
-    perform_request(crash);
+    perform_request(std::move(crash));
   }
-
-  return;
-
-fail:
-  delete crash;
 }
 
-static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) {
+static void crash_completed(borrowed_fd sockfd, std::unique_ptr<Crash> crash) {
-  ssize_t rc;
-  Crash* crash = static_cast<Crash*>(arg);
   TombstonedCrashPacket request = {};
 
-  CrashQueue::for_crash(crash)->on_crash_completed();
+  ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd.get(), &request, sizeof(request)));
-
-  if ((ev & EV_READ) == 0) {
-    goto fail;
-  }
-
-  rc = TEMP_FAILURE_RETRY(read(sockfd, &request, sizeof(request)));
   if (rc == -1) {
     PLOG(WARNING) << "failed to read from crash socket";
-    goto fail;
+    return;
   } else if (rc != sizeof(request)) {
     LOG(WARNING) << "crash socket received short read of length " << rc << " (expected "
                  << sizeof(request) << ")";
-    goto fail;
+    return;
   }
 
   if (request.packet_type != CrashPacketType::kCompletedDump) {
     LOG(WARNING) << "unexpected crash packet type, expected kCompletedDump, received "
                  << uint32_t(request.packet_type);
-    goto fail;
+    return;
   }
 
   if (crash->crash_tombstone_fd != -1) {
@@ -369,7 +361,7 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) {
     int rc = unlink(tombstone_path.c_str());
     if (rc != 0 && errno != ENOENT) {
       PLOG(ERROR) << "failed to unlink tombstone at " << tombstone_path;
-      goto fail;
+      return;
     }
 
     rc = linkat(AT_FDCWD, fd_path.c_str(), AT_FDCWD, tombstone_path.c_str(), AT_SYMLINK_FOLLOW);
@@ -394,10 +386,17 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) {
       }
     }
   }
+}
 
-fail:
+static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) {
+  std::unique_ptr<Crash> crash(static_cast<Crash*>(arg));
   CrashQueue* queue = CrashQueue::for_crash(crash);
-  delete crash;
+
+  CrashQueue::for_crash(crash)->on_crash_completed();
+
+  if ((ev & EV_READ) == EV_READ) {
+    crash_completed(sockfd, std::move(crash));
+  }
 
   // If there's something queued up, let them proceed.
   queue->maybe_dequeue_crashes(perform_request);