diff --git a/debuggerd/common/include/dump_type.h b/debuggerd/common/include/dump_type.h index a3e171b25..82ef7b67c 100644 --- a/debuggerd/common/include/dump_type.h +++ b/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) { - switch (rhs) { +inline const char* get_dump_type_name(const DebuggerdDumpType& dump_type) { + switch (dump_type) { case kDebuggerdNativeBacktrace: - stream << "kDebuggerdNativeBacktrace"; - break; + return "kDebuggerdNativeBacktrace"; case kDebuggerdTombstone: - stream << "kDebuggerdTombstone"; - break; + return "kDebuggerdTombstone"; case kDebuggerdJavaBacktrace: - stream << "kDebuggerdJavaBacktrace"; - break; + return "kDebuggerdJavaBacktrace"; case kDebuggerdAnyIntercept: - stream << "kDebuggerdAnyIntercept"; - break; + return "kDebuggerdAnyIntercept"; case kDebuggerdTombstoneProto: - stream << "kDebuggerdTombstoneProto"; - break; + return "kDebuggerdTombstoneProto"; default: - stream << "[unknown]"; + return "[unknown]"; } +} +inline std::ostream& operator<<(std::ostream& stream, const DebuggerdDumpType& rhs) { + stream << get_dump_type_name(rhs); return stream; } diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 19ff7ebf0..c3e1302b9 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/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)) { - FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(response) + } else if (rc != 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; - tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd, &status, intercept_type); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd, &response, intercept_type); + 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; - tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone); + 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; - tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone); + 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; - tombstoned_intercept(pid, &intercept_fd, &output_fd, &status, kDebuggerdTombstone); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(pid, &intercept_fd, &output_fd, &response, kDebuggerdTombstone); + 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; - tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(self, &intercept_fd, &output_fd, &response, kDebuggerdJavaBacktrace); + 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; - tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdJavaBacktrace); + 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); - ASSERT_EQ(InterceptStatus::kFailedAlreadyRegistered, status); + tombstoned_intercept(fake_pid, &intercept_fd_2, &output_fd_2, &response, + 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(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; - tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &status, kDebuggerdNativeBacktrace); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + tombstoned_intercept(fake_pid, &intercept_fd, &output_fd, &response, kDebuggerdNativeBacktrace); + 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 { @@ -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); - ASSERT_EQ(InterceptStatus::kRegistered, status); + InterceptResponse response = {}; + 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( diff --git a/debuggerd/protocol.h b/debuggerd/protocol.h index b60cf5bb6..212d6dc28 100644 --- a/debuggerd/protocol.h +++ b/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. diff --git a/debuggerd/tombstoned/intercept_manager.cpp b/debuggerd/tombstoned/intercept_manager.cpp index 613e6f57d..ac7b43132 100644 --- a/debuggerd/tombstoned/intercept_manager.cpp +++ b/debuggerd/tombstoned/intercept_manager.cpp @@ -19,7 +19,10 @@ #include #include +#include +#include #include +#include #include #include @@ -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(arg); - InterceptManager* intercept_manager = intercept->intercept_manager; + std::unique_ptr intercept(reinterpret_cast(arg)); 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; - } else { - auto it = intercept_manager->intercepts.find(intercept->intercept_pid); - 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); + LOG(WARNING) << "intercept for pid " << intercept->pid << " and type " << intercept->dump_type + << " closed before being registered."; + return; } + + 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) { - if (request.pid <= 0 || request.pid > std::numeric_limits::max()) { - return false; +void InterceptManager::Unregister(Intercept* intercept) { + CHECK(intercept->registered); + 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) { - return false; + dump_type_hash.erase(dump_type_entry); + 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(arg); + std::unique_ptr intercept(reinterpret_cast(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; - InterceptRequest intercept_request; - ssize_t result = - ReceiveFileDescriptors(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd); + unique_fd rcv_fd; + InterceptRequest intercept_request; + ssize_t result = + ReceiveFileDescriptors(sockfd, &intercept_request, sizeof(intercept_request), &rcv_fd); - if (result == -1) { - PLOG(WARNING) << "failed to read from intercept socket"; - goto fail; - } else if (result != sizeof(intercept_request)) { - LOG(WARNING) << "intercept socket received short read of length " << result << " (expected " - << sizeof(intercept_request) << ")"; - goto fail; - } - - // 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->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); + if (result == -1) { + PLOG(WARNING) << "failed to read from intercept socket"; + return; + } + if (result != sizeof(intercept_request)) { + LOG(WARNING) << "intercept socket received short read of length " << result << " (expected " + << sizeof(intercept_request) << ")"; + return; } - 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: - delete intercept; + // See if we can properly register the 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) { - if (interceptor == dump) { - return true; +static DebuggerdDumpType canonical_dump_type(const DebuggerdDumpType dump_type) { + // kDebuggerdTombstone and kDebuggerdTombstoneProto should be treated as + // a single dump_type for intercepts (kDebuggerdTombstone). + if (dump_type == kDebuggerdTombstoneProto) { + return kDebuggerdTombstone; } - - if (interceptor == kDebuggerdTombstone && dump == kDebuggerdTombstoneProto) { - return true; - } - - return false; + return dump_type; } -bool InterceptManager::GetIntercept(pid_t pid, DebuggerdDumpType dump_type, - android::base::unique_fd* out_fd) { - auto it = this->intercepts.find(pid); - if (it == this->intercepts.end()) { +Intercept* InterceptManager::Get(const pid_t pid, const DebuggerdDumpType dump_type) { + auto pid_entry = intercepts.find(pid); + if (pid_entry == 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::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) { - LOG(INFO) << "found registered intercept of type " << it->second->dump_type - << " for requested type kDebuggerdAnyIntercept"; - } else if (!dump_types_compatible(it->second->dump_type, dump_type)) { - LOG(WARNING) << "found non-matching intercept of type " << it->second->dump_type - << " for requested type: " << dump_type; + if (Get(request.pid, request.dump_type) != nullptr) { + response.status = InterceptStatus::kFailedAlreadyRegistered; + snprintf(response.error_message, sizeof(response.error_message), + "pid %" PRId32 " already registered, type %s", request.pid, + get_dump_type_name(request.dump_type)); return false; } - auto intercept = std::move(it->second); - this->intercepts.erase(it); + return true; +} + +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; } diff --git a/debuggerd/tombstoned/intercept_manager.h b/debuggerd/tombstoned/intercept_manager.h index a11d565a3..dc13aa9c0 100644 --- a/debuggerd/tombstoned/intercept_manager.h +++ b/debuggerd/tombstoned/intercept_manager.h @@ -28,17 +28,17 @@ #include "dump_type.h" struct InterceptManager; +struct InterceptRequest; +struct InterceptResponse; struct Intercept { - ~Intercept() { - event_free(intercept_event); - } + ~Intercept(); 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> intercepts; + std::unordered_map> 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); }; diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index 50558f7a3..cf7904f81 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/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) { unique_fd output_fd; - bool intercepted = - intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd); - if (intercepted) { + if (intercept_manager->FindIntercept(crash->crash_pid, crash->crash_type, &output_fd)) { if (crash->crash_type == kDebuggerdTombstoneProto) { crash->output.proto = CrashArtifact::devnull(); }