Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 69210a64 authored by Christopher Ferris's avatar Christopher Ferris
Browse files

Do not encode newline characters for abort/log. 

The isprint call will return false for chars such as newlines, which
means that the values '\n' and '\t' get encoded which was not the
intent of encodeding the abort and log messages. Add two oct_encode
versions, one that encodes all non-ascii values and only non-printable
ascii values for abort and log data. The other encoding function encodes
all chars that fail isprint() which is used for extra crash data.

Add new unit tests to verify that characters like newlines are not
encoded in the right places.

Bug: 381259755

Test: All unit tests pass.
Change-Id: I682f10e13a2e80ddfa7e87dfdf8181342eb22374
parent 43772f2b

debuggerd/debuggerd_test.cpp

+68 −9
Original line number Original line Diff line number Diff line
@@ -3303,8 +3303,44 @@ TEST_F(CrasherTest, log_with_newline) { 
  ASSERT_MATCH(result, ":\\s*This is on the next line.");

  ASSERT_MATCH(result, ":\\s*This is on the next line.");

}

}





TEST_F(CrasherTest, log_with_non_utf8) {

TEST_F(CrasherTest, log_with_non_printable_ascii_verify_encoded) {

  StartProcess([]() { LOG(FATAL) << "Invalid UTF-8: \xA0\xB0\xC0\xD0 and some other data."; });

  static const std::string kEncodedStr =

      "\x5C\x31"

      "\x5C\x32"

      "\x5C\x33"

      "\x5C\x34"

      "\x5C\x35"

      "\x5C\x36"

      "\x5C\x37"

      "\x5C\x31\x30"

      "\x5C\x31\x36"

      "\x5C\x31\x37"

      "\x5C\x32\x30"

      "\x5C\x32\x31"

      "\x5C\x32\x32"

      "\x5C\x32\x33"

      "\x5C\x32\x34"

      "\x5C\x32\x35"

      "\x5C\x32\x36"

      "\x5C\x32\x37"

      "\x5C\x33\x30"

      "\x5C\x33\x31"

      "\x5C\x33\x32"

      "\x5C\x33\x33"

      "\x5C\x33\x34"

      "\x5C\x33\x35"

      "\x5C\x33\x36"

      "\x5C\x33\x37"

      "\x5C\x31\x37\x37"

      "\x5C\x32\x34\x30"

      "\x5C\x32\x36\x30"

      "\x5C\x33\x30\x30"

      "\x5C\x33\x32\x30";

  StartProcess([]() {

    LOG(FATAL) << "Encoded: "

                  "\x1\x2\x3\x4\x5\x6\x7\x8\xe\xf\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b"

                  "\x1c\x1d\x1e\x1f\x7f\xA0\xB0\xC0\xD0 after";

  });





  unique_fd output_fd;

  unique_fd output_fd;

  StartIntercept(&output_fd);

  StartIntercept(&output_fd);
@@ -3317,15 +3353,38 @@ TEST_F(CrasherTest, log_with_non_utf8) { 
  std::string result;

  std::string result;

  ConsumeFd(std::move(output_fd), &result);

  ConsumeFd(std::move(output_fd), &result);

  // Verify the abort message is sanitized properly.

  // Verify the abort message is sanitized properly.

  size_t pos = result.find(

  size_t pos = result.find(std::string("Abort message: 'Encoded: ") + kEncodedStr + " after'");

      "Abort message: 'Invalid UTF-8: "

      "\x5C\x32\x34\x30\x5C\x32\x36\x30\x5C\x33\x30\x30\x5C\x33\x32\x30 and some other data.'");

  EXPECT_TRUE(pos != std::string::npos) << "Couldn't find sanitized abort message: " << result;

  EXPECT_TRUE(pos != std::string::npos) << "Couldn't find sanitized abort message: " << result;





  // Make sure that the log message is sanitized properly too.

  // Make sure that the log message is sanitized properly too.

  EXPECT_TRUE(

  EXPECT_TRUE(result.find(std::string("Encoded: ") + kEncodedStr + " after", pos + 1) !=

      result.find("Invalid UTF-8: \x5C\x32\x34\x30\x5C\x32\x36\x30\x5C\x33\x30\x30\x5C\x33\x32\x30 "

              std::string::npos)

                  "and some other data.",

      << "Couldn't find sanitized log message: " << result;

                  pos + 30) != std::string::npos)

}



TEST_F(CrasherTest, log_with_with_special_printable_ascii) {

  static const std::string kMsg = "Not encoded: \t\v\f\r\n after";

  StartProcess([]() { LOG(FATAL) << kMsg; });



  unique_fd output_fd;

  StartIntercept(&output_fd);

  FinishCrasher();

  AssertDeath(SIGABRT);

  int intercept_result;

  FinishIntercept(&intercept_result);

  ASSERT_EQ(1, intercept_result) << "tombstoned reported failure";



  std::string result;

  ConsumeFd(std::move(output_fd), &result);

  // Verify the abort message does not remove characters that are UTF8 but

  // are, technically, not printable.

  size_t pos = result.find(std::string("Abort message: '") + kMsg + "'");

  EXPECT_TRUE(pos != std::string::npos) << "Couldn't find abort message: " << result;



  // Make sure that the log message is handled properly too.

  // The logger automatically splits a newline message into two pieces.

  pos = result.find("Not encoded: \t\v\f\r", pos + kMsg.size());

  EXPECT_TRUE(pos != std::string::npos) << "Couldn't find log message: " << result;

  EXPECT_TRUE(result.find(" after", pos + 1) != std::string::npos)

      << "Couldn't find sanitized log message: " << result;

      << "Couldn't find sanitized log message: " << result;

}
 
}

debuggerd/libdebuggerd/include/libdebuggerd/utility_host.h

+4 −1
Original line number Original line Diff line number Diff line
@@ -30,4 +30,7 @@ constexpr size_t kTagGranuleSize = 16; 
constexpr size_t kNumTagColumns = 16;

constexpr size_t kNumTagColumns = 16;

constexpr size_t kNumTagRows = 16;

constexpr size_t kNumTagRows = 16;





std::string oct_encode(const std::string& data);
 
// Encode all non-ascii values and also ascii values that are not printable.

std::string oct_encode_non_ascii_printable(const std::string& data);

// Encode any value that fails isprint(), includes encoding chars like '\n' and '\t'.

std::string oct_encode_non_printable(const std::string& data);

debuggerd/libdebuggerd/tombstone_proto.cpp

+2 −2
Original line number Original line Diff line number Diff line
@@ -467,7 +467,7 @@ static void dump_abort_message(Tombstone* tombstone, 
  msg.resize(index);

  msg.resize(index);





  // Make sure only UTF8 characters are present since abort_message is a string.

  // Make sure only UTF8 characters are present since abort_message is a string.

  tombstone->set_abort_message(oct_encode(msg));

  tombstone->set_abort_message(oct_encode_non_ascii_printable(msg));

}

}





static void dump_open_fds(Tombstone* tombstone, const OpenFilesList* open_files) {

static void dump_open_fds(Tombstone* tombstone, const OpenFilesList* open_files) {
@@ -776,7 +776,7 @@ static void dump_log_file(Tombstone* tombstone, const char* logger, pid_t pid) { 
      log_msg->set_priority(prio);

      log_msg->set_priority(prio);

      log_msg->set_tag(tag);

      log_msg->set_tag(tag);

      // Make sure only UTF8 characters are present since message is a string.

      // Make sure only UTF8 characters are present since message is a string.

      log_msg->set_message(oct_encode(msg));

      log_msg->set_message(oct_encode_non_ascii_printable(msg));

    } while ((msg = nl));

    } while ((msg = nl));

  }

  }

  android_logger_list_free(logger_list);

  android_logger_list_free(logger_list);

debuggerd/libdebuggerd/tombstone_proto_to_text.cpp

+3 −2
Original line number Original line Diff line number Diff line
@@ -17,6 +17,7 @@ 
#include <libdebuggerd/tombstone_proto_to_text.h>

#include <libdebuggerd/tombstone_proto_to_text.h>

#include <libdebuggerd/utility_host.h>

#include <libdebuggerd/utility_host.h>





#include <ctype.h>

#include <inttypes.h>

#include <inttypes.h>





#include <algorithm>

#include <algorithm>
@@ -463,8 +464,8 @@ static void print_main_thread(CallbackType callback, SymbolizeCallbackType symbo 
  }

  }





  for (const auto& crash_detail : tombstone.crash_details()) {

  for (const auto& crash_detail : tombstone.crash_details()) {

    std::string oct_encoded_name = oct_encode(crash_detail.name());

    std::string oct_encoded_name = oct_encode_non_printable(crash_detail.name());

    std::string oct_encoded_data = oct_encode(crash_detail.data());

    std::string oct_encoded_data = oct_encode_non_printable(crash_detail.data());

    CBL("Extra crash detail: %s: '%s'", oct_encoded_name.c_str(), oct_encoded_data.c_str());

    CBL("Extra crash detail: %s: '%s'", oct_encoded_name.c_str(), oct_encoded_data.c_str());

  }

  }

debuggerd/libdebuggerd/utility_host.cpp

+13 −4
Original line number Original line Diff line number Diff line
@@ -16,6 +16,7 @@ 




#include "libdebuggerd/utility_host.h"

#include "libdebuggerd/utility_host.h"





#include <ctype.h>

#include <sys/prctl.h>

#include <sys/prctl.h>





#include <charconv>

#include <charconv>
@@ -102,23 +103,31 @@ std::string describe_pac_enabled_keys(long value) { 
  return describe_end(value, desc);

  return describe_end(value, desc);

}

}





std::string oct_encode(const std::string& data) {

static std::string oct_encode(const std::string& data, bool (*should_encode_func)(int)) {

  std::string oct_encoded;

  std::string oct_encoded;

  oct_encoded.reserve(data.size());

  oct_encoded.reserve(data.size());





  // N.B. the unsigned here is very important, otherwise e.g. \255 would render as

  // N.B. the unsigned here is very important, otherwise e.g. \255 would render as

  // \-123 (and overflow our buffer).

  // \-123 (and overflow our buffer).

  for (unsigned char c : data) {

  for (unsigned char c : data) {

    if (isprint(c)) {

    if (should_encode_func(c)) {

      oct_encoded += c;

    } else {

      std::string oct_digits("\\\0\0\0", 4);

      std::string oct_digits("\\\0\0\0", 4);

      // char is encodable in 3 oct digits

      // char is encodable in 3 oct digits

      static_assert(std::numeric_limits<unsigned char>::max() <= 8 * 8 * 8);

      static_assert(std::numeric_limits<unsigned char>::max() <= 8 * 8 * 8);

      auto [ptr, ec] = std::to_chars(oct_digits.data() + 1, oct_digits.data() + 4, c, 8);

      auto [ptr, ec] = std::to_chars(oct_digits.data() + 1, oct_digits.data() + 4, c, 8);

      oct_digits.resize(ptr - oct_digits.data());

      oct_digits.resize(ptr - oct_digits.data());

      oct_encoded += oct_digits;

      oct_encoded += oct_digits;

    } else {

      oct_encoded += c;

    }

    }

  }

  }

  return oct_encoded;

  return oct_encoded;

}

}



std::string oct_encode_non_ascii_printable(const std::string& data) {

  return oct_encode(data, [](int c) { return !isgraph(c) && !isspace(c); });

}



std::string oct_encode_non_printable(const std::string& data) {

  return oct_encode(data, [](int c) { return !isprint(c); });

}