diff --git a/debuggerd/client/debuggerd_client.cpp b/debuggerd/client/debuggerd_client.cpp index b9fb512f1..224444f1f 100644 --- a/debuggerd/client/debuggerd_client.cpp +++ b/debuggerd/client/debuggerd_client.cpp @@ -65,24 +65,25 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du auto time_left = [timeout_ms, &end]() { return end - std::chrono::steady_clock::now(); }; auto set_timeout = [timeout_ms, &time_left](int sockfd) { if (timeout_ms <= 0) { - return true; + return -1; } auto remaining = time_left(); if (remaining < decltype(remaining)::zero()) { - return false; + LOG(ERROR) << "timeout expired"; + return -1; } struct timeval timeout; populate_timeval(&timeout, remaining); if (setsockopt(sockfd, SOL_SOCKET, SO_RCVTIMEO, &timeout, sizeof(timeout)) != 0) { - return false; + return -1; } if (setsockopt(sockfd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)) != 0) { - return false; + return -1; } - return true; + return sockfd; }; sockfd.reset(socket(AF_LOCAL, SOCK_SEQPACKET, 0)); @@ -91,12 +92,7 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du return false; } - if (!set_timeout(sockfd)) { - PLOG(ERROR) << "libdebugger_client: failed to set timeout"; - return false; - } - - if (socket_local_client_connect(sockfd.get(), kTombstonedInterceptSocketName, + if (socket_local_client_connect(set_timeout(sockfd.get()), kTombstonedInterceptSocketName, ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET) == -1) { PLOG(ERROR) << "libdebuggerd_client: failed to connect to tombstoned"; return false; @@ -115,21 +111,35 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du return false; } - if (send_fd(sockfd.get(), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) { + if (send_fd(set_timeout(sockfd), &req, sizeof(req), std::move(pipe_write)) != sizeof(req)) { PLOG(ERROR) << "libdebuggerd_client: failed to send output fd to tombstoned"; return false; } + // Check to make sure we've successfully registered. + InterceptResponse response; + ssize_t rc = + TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC)); + if (rc == 0) { + LOG(ERROR) << "libdebuggerd_client: failed to read response from tombstoned: timeout reached?"; + return false; + } else if (rc != sizeof(response)) { + LOG(ERROR) + << "libdebuggerd_client: received packet of unexpected length from tombstoned: expected " + << sizeof(response) << ", received " << rc; + return false; + } + + if (response.status != InterceptStatus::kRegistered) { + LOG(ERROR) << "libdebuggerd_client: unexpected registration response: " + << static_cast(response.status); + return false; + } + bool backtrace = dump_type == kDebuggerdBacktrace; send_signal(pid, backtrace); - if (!set_timeout(sockfd)) { - PLOG(ERROR) << "libdebuggerd_client: failed to set timeout"; - return false; - } - - InterceptResponse response; - ssize_t rc = TEMP_FAILURE_RETRY(recv(sockfd.get(), &response, sizeof(response), MSG_TRUNC)); + rc = TEMP_FAILURE_RETRY(recv(set_timeout(sockfd.get()), &response, sizeof(response), MSG_TRUNC)); if (rc == 0) { LOG(ERROR) << "libdebuggerd_client: failed to read response from tombstoned: timeout reached?"; return false; @@ -140,7 +150,7 @@ bool debuggerd_trigger_dump(pid_t pid, unique_fd output_fd, DebuggerdDumpType du return false; } - if (response.success != 1) { + if (response.status != InterceptStatus::kStarted) { response.error_message[sizeof(response.error_message) - 1] = '\0'; LOG(ERROR) << "libdebuggerd_client: tombstoned reported failure: " << response.error_message; return false; diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 1a27f3f6a..410c4ae59 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -77,6 +77,54 @@ constexpr char kWaitForGdbKey[] = "debug.debuggerd.wait_for_gdb"; } \ } while (0) +static void tombstoned_intercept(pid_t target_pid, unique_fd* intercept_fd, unique_fd* output_fd) { + intercept_fd->reset(socket_local_client(kTombstonedInterceptSocketName, + ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET)); + if (intercept_fd->get() == -1) { + FAIL() << "failed to contact tombstoned: " << strerror(errno); + } + + InterceptRequest req = {.pid = target_pid}; + + unique_fd output_pipe_write; + if (!Pipe(output_fd, &output_pipe_write)) { + FAIL() << "failed to create output pipe: " << strerror(errno); + } + + std::string pipe_size_str; + int pipe_buffer_size; + if (!android::base::ReadFileToString("/proc/sys/fs/pipe-max-size", &pipe_size_str)) { + FAIL() << "failed to read /proc/sys/fs/pipe-max-size: " << strerror(errno); + } + + pipe_size_str = android::base::Trim(pipe_size_str); + + if (!android::base::ParseInt(pipe_size_str.c_str(), &pipe_buffer_size, 0)) { + FAIL() << "failed to parse pipe max size"; + } + + if (fcntl(output_fd->get(), F_SETPIPE_SZ, pipe_buffer_size) != pipe_buffer_size) { + FAIL() << "failed to set pipe size: " << strerror(errno); + } + + if (send_fd(intercept_fd->get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) { + FAIL() << "failed to send output fd to tombstoned: " << strerror(errno); + } + + InterceptResponse response; + ssize_t 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) + << ", received " << rc; + } + + ASSERT_EQ(InterceptStatus::kRegistered, response.status); +} + class CrasherTest : public ::testing::Test { public: pid_t crasher_pid = -1; @@ -118,38 +166,7 @@ void CrasherTest::StartIntercept(unique_fd* output_fd) { FAIL() << "crasher hasn't been started"; } - intercept_fd.reset(socket_local_client(kTombstonedInterceptSocketName, - ANDROID_SOCKET_NAMESPACE_RESERVED, SOCK_SEQPACKET)); - if (intercept_fd == -1) { - FAIL() << "failed to contact tombstoned: " << strerror(errno); - } - - InterceptRequest req = {.pid = crasher_pid }; - - unique_fd output_pipe_write; - if (!Pipe(output_fd, &output_pipe_write)) { - FAIL() << "failed to create output pipe: " << strerror(errno); - } - - std::string pipe_size_str; - int pipe_buffer_size; - if (!android::base::ReadFileToString("/proc/sys/fs/pipe-max-size", &pipe_size_str)) { - FAIL() << "failed to read /proc/sys/fs/pipe-max-size: " << strerror(errno); - } - - pipe_size_str = android::base::Trim(pipe_size_str); - - if (!android::base::ParseInt(pipe_size_str.c_str(), &pipe_buffer_size, 0)) { - FAIL() << "failed to parse pipe max size"; - } - - if (fcntl(output_fd->get(), F_SETPIPE_SZ, pipe_buffer_size) != pipe_buffer_size) { - FAIL() << "failed to set pipe size: " << strerror(errno); - } - - if (send_fd(intercept_fd.get(), &req, sizeof(req), std::move(output_pipe_write)) != sizeof(req)) { - FAIL() << "failed to send output fd to tombstoned: " << strerror(errno); - } + tombstoned_intercept(crasher_pid, &this->intercept_fd, output_fd); } void CrasherTest::FinishIntercept(int* result) { @@ -165,7 +182,7 @@ void CrasherTest::FinishIntercept(int* result) { FAIL() << "received packet of unexpected length from tombstoned: expected " << sizeof(response) << ", received " << rc; } else { - *result = response.success; + *result = response.status == InterceptStatus::kStarted ? 1 : 0; } } diff --git a/debuggerd/include/debuggerd/protocol.h b/debuggerd/include/debuggerd/protocol.h index bb2ab0dc9..0756876d8 100644 --- a/debuggerd/include/debuggerd/protocol.h +++ b/debuggerd/include/debuggerd/protocol.h @@ -56,8 +56,14 @@ struct InterceptRequest { int32_t pid; }; +enum class InterceptStatus : uint8_t { + kFailed, + kStarted, + kRegistered, +}; + // Sent either immediately upon failure, or when the intercept has been used. struct InterceptResponse { - uint8_t success; // 0 or 1 + InterceptStatus status; char error_message[127]; // always null-terminated }; diff --git a/debuggerd/tombstoned/intercept_manager.cpp b/debuggerd/tombstoned/intercept_manager.cpp index 789260d07..dff942cc9 100644 --- a/debuggerd/tombstoned/intercept_manager.cpp +++ b/debuggerd/tombstoned/intercept_manager.cpp @@ -105,6 +105,7 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { // We trust the other side, so only do minimal validity checking. if (intercept_request.pid <= 0 || intercept_request.pid > std::numeric_limits::max()) { InterceptResponse response = {}; + response.status = InterceptStatus::kFailed; snprintf(response.error_message, sizeof(response.error_message), "invalid pid %" PRId32, intercept_request.pid); TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response))); @@ -113,9 +114,10 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { intercept->intercept_pid = intercept_request.pid; - // Register the intercept with the InterceptManager. + // Check if it's already registered. if (intercept_manager->intercepts.count(intercept_request.pid) > 0) { InterceptResponse response = {}; + response.status = InterceptStatus::kFailed; snprintf(response.error_message, sizeof(response.error_message), "pid %" PRId32 " already intercepted", intercept_request.pid); TEMP_FAILURE_RETRY(write(sockfd, &response, sizeof(response))); @@ -123,6 +125,15 @@ static void intercept_request_cb(evutil_socket_t sockfd, short ev, void* arg) { 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; @@ -174,7 +185,7 @@ bool InterceptManager::GetIntercept(pid_t pid, android::base::unique_fd* out_fd) LOG(INFO) << "found intercept fd " << intercept->output_fd.get() << " for pid " << pid; InterceptResponse response = {}; - response.success = 1; + response.status = InterceptStatus::kStarted; TEMP_FAILURE_RETRY(write(intercept->sockfd, &response, sizeof(response))); *out_fd = std::move(intercept->output_fd);