From 3a0833c9cdf3b8c09a41bf5cadd9ffe8dd135c84 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris@google.com>
Date: Fri, 28 Jul 2023 13:07:53 -0700
Subject: [PATCH] Fix potential miscellaneous debuggerd issues.

Check for the log opening failing.

Add the ability to put error messages in the log and tombstone so
that it's clear if the log reading failed in some way.

Adjust test so that if there is a log or if no log exists, the test
will still pass.

Print an <unknown> if the command line is unreadable instead of nothing.

Test: Ran unit tests.
Test: Induced error and verified error message is save in tombstone.
Change-Id: I2fce8078573b40b9fed3cd453235f3824cadb5e3
---
 debuggerd/debuggerd_test.cpp                  | 10 ++++--
 debuggerd/libdebuggerd/tombstone_proto.cpp    | 35 +++++++++++++++----
 .../libdebuggerd/tombstone_proto_to_text.cpp  |  2 ++
 3 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp
index 52c1c2554..19ff7ebf0 100644
--- a/debuggerd/debuggerd_test.cpp
+++ b/debuggerd/debuggerd_test.cpp
@@ -2264,10 +2264,14 @@ TEST_F(CrasherTest, fault_address_after_last_map) {
 
   ASSERT_MATCH(result, R"(\nmemory map \(.*\): \(fault address prefixed with --->)\n)");
 
-  // Assumes that the open files section comes after the map section.
-  // If that assumption changes, the regex below needs to change.
+  // Verifies that the fault address error message is at the end of the
+  // maps section. To do this, the check below looks for the start of the
+  // open files section or the start of the log file section. It's possible
+  // for either of these sections to be present after the maps section right
+  // now.
+  // If the sections move around, this check might need to be modified.
   match_str = android::base::StringPrintf(
-      R"(\n--->Fault address falls at %s after any mapped regions\n\nopen files:)",
+      R"(\n--->Fault address falls at %s after any mapped regions\n(---------|\nopen files:))",
       format_pointer(crash_uptr).c_str());
   ASSERT_MATCH(result, match_str);
 }
diff --git a/debuggerd/libdebuggerd/tombstone_proto.cpp b/debuggerd/libdebuggerd/tombstone_proto.cpp
index 7b2e0689e..744bfabf5 100644
--- a/debuggerd/libdebuggerd/tombstone_proto.cpp
+++ b/debuggerd/libdebuggerd/tombstone_proto.cpp
@@ -493,27 +493,48 @@ static void dump_mappings(Tombstone* tombstone, unwindstack::Maps* maps,
   }
 }
 
+// This creates a fake log message that indicates an error occurred when
+// reading the log.
+static void add_error_log_msg(Tombstone* tombstone, const std::string&& error_msg) {
+  LogBuffer buffer;
+  buffer.set_name("ERROR");
+
+  LogMessage* log_msg = buffer.add_logs();
+  log_msg->set_timestamp("00-00 00:00:00.000");
+  log_msg->set_pid(0);
+  log_msg->set_tid(0);
+  log_msg->set_priority(ANDROID_LOG_ERROR);
+  log_msg->set_tag("");
+  log_msg->set_message(error_msg);
+
+  *tombstone->add_log_buffers() = std::move(buffer);
+
+  async_safe_format_log(ANDROID_LOG_ERROR, LOG_TAG, "%s", error_msg.c_str());
+}
+
 static void dump_log_file(Tombstone* tombstone, const char* logger, pid_t pid) {
   logger_list* logger_list = android_logger_list_open(android_name_to_log_id(logger),
                                                       ANDROID_LOG_NONBLOCK, kMaxLogMessages, pid);
+  if (logger_list == nullptr) {
+    add_error_log_msg(tombstone, android::base::StringPrintf("Cannot open log file %s", logger));
+    return;
+  }
 
   LogBuffer buffer;
-
   while (true) {
     log_msg log_entry;
     ssize_t actual = android_logger_list_read(logger_list, &log_entry);
-
     if (actual < 0) {
       if (actual == -EINTR) {
         // interrupted by signal, retry
         continue;
       }
-      if (actual == -EAGAIN) {
-        // non-blocking EOF; we're done
-        break;
-      } else {
-        break;
+      // Don't consider EAGAIN an error since this is a non-blocking call.
+      if (actual != -EAGAIN) {
+        add_error_log_msg(tombstone, android::base::StringPrintf("reading log %s failed (%s)",
+                                                                 logger, strerror(-actual)));
       }
+      break;
     } else if (actual == 0) {
       break;
     }
diff --git a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp b/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp
index 8e6abdfa1..eed81fc15 100644
--- a/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp
+++ b/debuggerd/libdebuggerd/tombstone_proto_to_text.cpp
@@ -81,6 +81,8 @@ static void print_thread_header(CallbackType callback, const Tombstone& tombston
   if (!tombstone.command_line().empty()) {
     process_name = tombstone.command_line()[0].c_str();
     CB(should_log, "Cmdline: %s", android::base::Join(tombstone.command_line(), " ").c_str());
+  } else {
+    CB(should_log, "Cmdline: <unknown>");
   }
   CB(should_log, "pid: %d, tid: %d, name: %s  >>> %s <<<", tombstone.pid(), thread.id(),
      thread.name().c_str(), process_name);