From eda96eddcbdda9632166232b2363c7b84da0994d Mon Sep 17 00:00:00 2001 From: Jerome Gaillard Date: Tue, 26 Jan 2021 12:36:12 +0000 Subject: [PATCH] Revert "tombstoned: make it easier to add more types of outputs." Revert "Let crash_dump read /proc/$PID." Revert submission 1556807-tombstone_proto Reason for revert: b/178455196, Broken test: android.seccomp.cts.SeccompHostJUnit4DeviceTest#testAppZygoteSyscalls on git_master on cf_x86_64_phone-userdebug Reverted Changes: Ide6811297:tombstoned: switch from goto to RAII. I8d285c4b4:tombstoned: make it easier to add more types of ou... Id0f0fa285:tombstoned: support for protobuf fds. I6be6082ab:Let crash_dump read /proc/$PID. Id812ca390:Make protobuf vendor_ramdisk_available. Ieeece6e6d:libdebuggerd: add protobuf implementation. Change-Id: Ib2403c1b61f6cf0513b76361440fbc5909d7554a --- debuggerd/debuggerd_test.cpp | 4 +- debuggerd/tombstoned/tombstoned.cpp | 151 +++++++++------------------- 2 files changed, 48 insertions(+), 107 deletions(-) diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp index 191931da6..45e555f44 100644 --- a/debuggerd/debuggerd_test.cpp +++ b/debuggerd/debuggerd_test.cpp @@ -1309,11 +1309,11 @@ TEST(tombstoned, java_trace_intercept_smoke) { tombstoned_intercept(self, &intercept_fd, &output_fd, &status, kDebuggerdJavaBacktrace); ASSERT_EQ(InterceptStatus::kRegistered, status); - // First connect to tombstoned requesting a native tombstone. This + // First connect to tombstoned requesting a native backtrace. This // should result in a "regular" FD and not the installed intercept. const char native[] = "native"; unique_fd tombstoned_socket, input_fd; - ASSERT_TRUE(tombstoned_connect(self, &tombstoned_socket, &input_fd, kDebuggerdTombstone)); + ASSERT_TRUE(tombstoned_connect(self, &tombstoned_socket, &input_fd, kDebuggerdNativeBacktrace)); ASSERT_TRUE(android::base::WriteFully(input_fd.get(), native, sizeof(native))); tombstoned_notify_completion(tombstoned_socket.get()); diff --git a/debuggerd/tombstoned/tombstoned.cpp b/debuggerd/tombstoned/tombstoned.cpp index 2b74fd489..57e6b6c56 100644 --- a/debuggerd/tombstoned/tombstoned.cpp +++ b/debuggerd/tombstoned/tombstoned.cpp @@ -59,26 +59,14 @@ enum CrashStatus { kCrashStatusQueued, }; -struct CrashArtifact { - unique_fd fd; - std::optional temporary_path; -}; - -struct CrashArtifactPaths { - std::string text; -}; - -struct CrashOutput { - CrashArtifact text; -}; - // Ownership of Crash is a bit messy. // It's either owned by an active event that must have a timeout, or owned by // queued_requests, in the case that multiple crashes come in at the same time. struct Crash { ~Crash() { event_free(crash_event); } - CrashOutput output; + std::string crash_tombstone_path; + unique_fd crash_tombstone_fd; unique_fd crash_socket_fd; pid_t crash_pid; event* crash_event = nullptr; @@ -130,56 +118,29 @@ class CrashQueue { return &queue; } - CrashArtifact create_temporary_file() const { - CrashArtifact result; - - std::optional path; - result.fd.reset(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640)); - if (result.fd == -1) { + std::pair get_output() { + std::string path; + unique_fd result(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640)); + if (result == -1) { // We might not have O_TMPFILE. Try creating with an arbitrary filename instead. static size_t counter = 0; std::string tmp_filename = StringPrintf(".temporary%zu", counter++); - result.fd.reset(openat(dir_fd_, tmp_filename.c_str(), - O_WRONLY | O_APPEND | O_CREAT | O_TRUNC | O_CLOEXEC, 0640)); - if (result.fd == -1) { + result.reset(openat(dir_fd_, tmp_filename.c_str(), + O_WRONLY | O_APPEND | O_CREAT | O_TRUNC | O_CLOEXEC, 0640)); + if (result == -1) { PLOG(FATAL) << "failed to create temporary tombstone in " << dir_path_; } - result.temporary_path = std::move(tmp_filename); + path = StringPrintf("%s/%s", dir_path_.c_str(), tmp_filename.c_str()); } - - return std::move(result); + return std::make_pair(std::move(path), std::move(result)); } - std::optional get_output(DebuggerdDumpType dump_type) { - CrashOutput result; - - switch (dump_type) { - case kDebuggerdNativeBacktrace: - case kDebuggerdJavaBacktrace: - // Don't generate tombstones for backtrace requests. - return {}; - - case kDebuggerdTombstone: - result.text = create_temporary_file(); - break; - - default: - LOG(ERROR) << "unexpected dump type: " << dump_type; - return {}; - } - - return result; - } - - borrowed_fd dir_fd() { return dir_fd_; } - - CrashArtifactPaths get_next_artifact_paths() { - CrashArtifactPaths result; - result.text = StringPrintf("%s%02d", file_name_prefix_.c_str(), next_artifact_); - + std::string get_next_artifact_path() { + std::string file_name = + StringPrintf("%s/%s%02d", dir_path_.c_str(), file_name_prefix_.c_str(), next_artifact_); next_artifact_ = (next_artifact_ + 1) % max_artifacts_; - return result; + return file_name; } // Consumes crash if it returns true, otherwise leaves it untouched. @@ -210,8 +171,7 @@ class CrashQueue { time_t oldest_time = std::numeric_limits::max(); for (size_t i = 0; i < max_artifacts_; ++i) { - std::string path = - StringPrintf("%s/%s%02zu", dir_path_.c_str(), file_name_prefix_.c_str(), i); + std::string path = StringPrintf("%s/%s%02zu", dir_path_.c_str(), file_name_prefix_.c_str(), i); struct stat st; if (stat(path.c_str(), &st) != 0) { if (errno == ENOENT) { @@ -252,8 +212,7 @@ class CrashQueue { static constexpr bool kJavaTraceDumpsEnabled = true; // Forward declare the callbacks so they can be placed in a sensible order. -static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int, - void*); +static void crash_accept_cb(evconnlistener* listener, evutil_socket_t sockfd, sockaddr*, int, void*); 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); @@ -262,20 +221,20 @@ static void perform_request(std::unique_ptr crash) { bool intercepted = intercept_manager->GetIntercept(crash->crash_pid, crash->crash_type, &output_fd); if (!intercepted) { - if (auto o = CrashQueue::for_crash(crash.get())->get_output(crash->crash_type); o) { - crash->output = std::move(*o); - output_fd.reset(dup(crash->output.text.fd)); + if (crash->crash_type == kDebuggerdNativeBacktrace) { + // Don't generate tombstones for native backtrace requests. + output_fd.reset(open("/dev/null", O_WRONLY | O_CLOEXEC)); } else { - LOG(ERROR) << "failed to get crash output for type " << crash->crash_type; - return; + std::tie(crash->crash_tombstone_path, output_fd) = CrashQueue::for_crash(crash)->get_output(); + crash->crash_tombstone_fd.reset(dup(output_fd.get())); } } - TombstonedCrashPacket response = {.packet_type = CrashPacketType::kPerformDump}; - + TombstonedCrashPacket response = { + .packet_type = CrashPacketType::kPerformDump + }; ssize_t rc = SendFileDescriptors(crash->crash_socket_fd, &response, sizeof(response), output_fd.get()); - output_fd.reset(); if (rc == -1) { @@ -375,20 +334,8 @@ static void crash_request_cb(evutil_socket_t sockfd, short ev, void* arg) { } } -static bool link_fd(borrowed_fd fd, borrowed_fd dirfd, const std::string& path) { - std::string fd_path = StringPrintf("/proc/self/fd/%d", fd.get()); - - int rc = linkat(AT_FDCWD, fd_path.c_str(), dirfd.get(), path.c_str(), AT_SYMLINK_FOLLOW); - if (rc != 0) { - PLOG(ERROR) << "failed to link file descriptor"; - return false; - } - return true; -} - static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { TombstonedCrashPacket request = {}; - CrashQueue* queue = CrashQueue::for_crash(crash); ssize_t rc = TEMP_FAILURE_RETRY(read(sockfd.get(), &request, sizeof(request))); if (rc == -1) { @@ -406,43 +353,37 @@ static void crash_completed(borrowed_fd sockfd, std::unique_ptr crash) { return; } - if (crash->output.text.fd == -1) { - LOG(WARNING) << "missing output fd"; - return; - } + if (crash->crash_tombstone_fd != -1) { + std::string fd_path = StringPrintf("/proc/self/fd/%d", crash->crash_tombstone_fd.get()); + std::string tombstone_path = CrashQueue::for_crash(crash)->get_next_artifact_path(); - CrashArtifactPaths paths = queue->get_next_artifact_paths(); + // linkat doesn't let us replace a file, so we need to unlink first. + int rc = unlink(tombstone_path.c_str()); + if (rc != 0 && errno != ENOENT) { + PLOG(ERROR) << "failed to unlink tombstone at " << tombstone_path; + return; + } - // Always try to unlink the tombstone file. - // linkat doesn't let us replace a file, so we need to unlink before linking - // our results onto disk, and if we fail for some reason, we should delete - // stale tombstones to avoid confusing inconsistency. - rc = unlinkat(queue->dir_fd().get(), paths.text.c_str(), 0); - if (rc != 0 && errno != ENOENT) { - PLOG(ERROR) << "failed to unlink tombstone at " << paths.text; - return; - } - - if (crash->output.text.fd != -1) { - if (!link_fd(crash->output.text.fd, queue->dir_fd(), paths.text)) { - LOG(ERROR) << "failed to link tombstone"; + rc = linkat(AT_FDCWD, fd_path.c_str(), AT_FDCWD, tombstone_path.c_str(), AT_SYMLINK_FOLLOW); + if (rc != 0) { + PLOG(ERROR) << "failed to link tombstone"; } else { if (crash->crash_type == kDebuggerdJavaBacktrace) { - LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << paths.text; + LOG(ERROR) << "Traces for pid " << crash->crash_pid << " written to: " << tombstone_path; } else { // NOTE: Several tools parse this log message to figure out where the // tombstone associated with a given native crash was written. Any changes // to this message must be carefully considered. - LOG(ERROR) << "Tombstone written to: " << paths.text; + LOG(ERROR) << "Tombstone written to: " << tombstone_path; } } - } - // If we don't have O_TMPFILE, we need to clean up after ourselves. - if (crash->output.text.temporary_path) { - rc = unlinkat(queue->dir_fd().get(), crash->output.text.temporary_path->c_str(), 0); - if (rc != 0) { - PLOG(ERROR) << "failed to unlink temporary tombstone at " << paths.text; + // If we don't have O_TMPFILE, we need to clean up after ourselves. + if (!crash->crash_tombstone_path.empty()) { + rc = unlink(crash->crash_tombstone_path.c_str()); + if (rc != 0) { + PLOG(ERROR) << "failed to unlink temporary tombstone at " << crash->crash_tombstone_path; + } } } } @@ -451,7 +392,7 @@ static void crash_completed_cb(evutil_socket_t sockfd, short ev, void* arg) { std::unique_ptr crash(static_cast(arg)); CrashQueue* queue = CrashQueue::for_crash(crash); - queue->on_crash_completed(); + CrashQueue::for_crash(crash)->on_crash_completed(); if ((ev & EV_READ) == EV_READ) { crash_completed(sockfd, std::move(crash));