From 10e428dd77181003942542d8de5c4615b693c610 Mon Sep 17 00:00:00 2001
From: Peter Collingbourne <pcc@google.com>
Date: Fri, 17 Jul 2020 14:49:31 -0700
Subject: [PATCH] Fix dumping of heap memory.

After r.android.com/1288984 we started failing to dump memory contents
for heap addresses because the tag started causing any addresses to
fail this bounds check. Add an untag_address() call to the bounds check
so that the tag is ignored.

Bug: 154272452
Change-Id: I3a6d1a078b21871bd93164150a123549f83289f6
---
 debuggerd/debuggerd_test.cpp       | 44 ++++++++++++++++++++++++++++++
 debuggerd/libdebuggerd/utility.cpp |  4 +++
 2 files changed, 48 insertions(+)

diff --git a/debuggerd/debuggerd_test.cpp b/debuggerd/debuggerd_test.cpp
index 9d7658eb0..f1119cc03 100644
--- a/debuggerd/debuggerd_test.cpp
+++ b/debuggerd/debuggerd_test.cpp
@@ -172,6 +172,8 @@ class CrasherTest : public ::testing::Test {
   void StartCrasher(const std::string& crash_type);
   void FinishCrasher();
   void AssertDeath(int signo);
+
+  static void Trap(void* ptr);
 };
 
 CrasherTest::CrasherTest() {
@@ -334,6 +336,48 @@ TEST_F(CrasherTest, tagged_fault_addr) {
       R"(signal 11 \(SIGSEGV\), code 1 \(SEGV_MAPERR\), fault addr (0x100000000000dead|0xdead))");
 }
 
+// Marked as weak to prevent the compiler from removing the malloc in the caller. In theory, the
+// compiler could still clobber the argument register before trapping, but that's unlikely.
+__attribute__((weak)) void CrasherTest::Trap(void* ptr ATTRIBUTE_UNUSED) {
+  __builtin_trap();
+}
+
+TEST_F(CrasherTest, heap_addr_in_register) {
+#if defined(__i386__)
+  GTEST_SKIP() << "architecture does not pass arguments in registers";
+#endif
+  int intercept_result;
+  unique_fd output_fd;
+  StartProcess([]() {
+    // Crash with a heap pointer in the first argument register.
+    Trap(malloc(1));
+  });
+
+  StartIntercept(&output_fd);
+  FinishCrasher();
+  int status;
+  ASSERT_EQ(crasher_pid, TIMEOUT(30, waitpid(crasher_pid, &status, 0)));
+  ASSERT_TRUE(WIFSIGNALED(status)) << "crasher didn't terminate via a signal";
+  // Don't test the signal number because different architectures use different signals for
+  // __builtin_trap().
+  FinishIntercept(&intercept_result);
+
+  ASSERT_EQ(1, intercept_result) << "tombstoned reported failure";
+
+  std::string result;
+  ConsumeFd(std::move(output_fd), &result);
+
+#if defined(__aarch64__)
+  ASSERT_MATCH(result, "memory near x0");
+#elif defined(__arm__)
+  ASSERT_MATCH(result, "memory near r0");
+#elif defined(__x86_64__)
+  ASSERT_MATCH(result, "memory near rdi");
+#else
+  ASSERT_TRUE(false) << "unsupported architecture";
+#endif
+}
+
 #if defined(__aarch64__) && defined(ANDROID_EXPERIMENTAL_MTE)
 static void SetTagCheckingLevelSync() {
   int tagged_addr_ctrl = prctl(PR_GET_TAGGED_ADDR_CTRL, 0, 0, 0, 0);
diff --git a/debuggerd/libdebuggerd/utility.cpp b/debuggerd/libdebuggerd/utility.cpp
index c8a3431b7..0a491bbf6 100644
--- a/debuggerd/libdebuggerd/utility.cpp
+++ b/debuggerd/libdebuggerd/utility.cpp
@@ -135,6 +135,10 @@ void dump_memory(log_t* log, unwindstack::Memory* memory, uint64_t addr, const s
     addr -= 32;
   }
 
+  // We don't want the address tag to interfere with the bounds check below or appear in the
+  // addresses in the memory dump.
+  addr = untag_address(addr);
+
   // Don't bother if the address looks too low, or looks too high.
   if (addr < 4096 ||
 #if defined(__LP64__)