From b92b52c0714c3fd3aa45e97f63d9f87df8d03498 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 16 Oct 2023 19:14:28 -0700 Subject: [PATCH] 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 | 24 +- debuggerd/debuggerd_test.cpp | 215 ++++++++++++--- debuggerd/protocol.h | 2 +- debuggerd/tombstoned/intercept_manager.cpp | 288 ++++++++++++--------- debuggerd/tombstoned/intercept_manager.h | 17 +- debuggerd/tombstoned/tombstoned.cpp | 4 +- 6 files changed, 367 insertions(+), 183 deletions(-) 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(); }