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
diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp
index 191931d..45e555f 100644
--- a/debuggerd/debuggerd_test.cpp
+++ b/debuggerd/debuggerd_test.cpp
@@ -1309,11 +1309,11 @@
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 2b74fd4..57e6b6c 100644
--- a/debuggerd/tombstoned/tombstoned.cpp
+++ b/debuggerd/tombstoned/tombstoned.cpp
@@ -59,26 +59,14 @@
kCrashStatusQueued,
};
-struct CrashArtifact {
- unique_fd fd;
- std::optional<std::string> 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 @@
return &queue;
}
- CrashArtifact create_temporary_file() const {
- CrashArtifact result;
-
- std::optional<std::string> path;
- result.fd.reset(openat(dir_fd_, ".", O_WRONLY | O_APPEND | O_TMPFILE | O_CLOEXEC, 0640));
- if (result.fd == -1) {
+ std::pair<std::string, unique_fd> 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<CrashOutput> 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 @@
time_t oldest_time = std::numeric_limits<time_t>::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 @@
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 @@
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 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> 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 @@
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 @@
std::unique_ptr<Crash> crash(static_cast<Crash*>(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));