From 065f15619528c95784db0c0193e8d36303e64b10 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris@google.com>
Date: Thu, 13 Dec 2018 09:33:45 -0800
Subject: [PATCH] Do not remove speculative frames in all cases.

If the first frame of an unwind is a totally invalid pc that's not in
any map, a speculative frame is added. Rather than deleting this frame
if no more unwinding is possible, leave it. This fixes a case where
the only frame you get is an invalid one, but the speculative frame
winds up in a shared library or somewhere else and gets removed.

Bug: 120505086

Test: New unit tests to catch this case pass.
Test: Verified original crashing program now emits two backtrace lines.
Change-Id: I088dff21c057386dcdaeb3fc2578b24322683bd0
---
 libunwindstack/Unwinder.cpp           | 10 ++++-
 libunwindstack/tests/UnwinderTest.cpp | 64 ++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/libunwindstack/Unwinder.cpp b/libunwindstack/Unwinder.cpp
index b3c55494a..792cd0b70 100644
--- a/libunwindstack/Unwinder.cpp
+++ b/libunwindstack/Unwinder.cpp
@@ -239,8 +239,14 @@ void Unwinder::Unwind(const std::vector<std::string>* initial_map_names_to_skip,
 
     if (!stepped) {
       if (return_address_attempt) {
-        // Remove the speculative frame.
-        frames_.pop_back();
+        // Only remove the speculative frame if there are more than two frames
+        // or the pc in the first frame is in a valid map.
+        // This allows for a case where the code jumps into the middle of
+        // nowhere, but there is no other unwind information after that.
+        if (frames_.size() != 2 || maps_->Find(frames_[0].pc) != nullptr) {
+          // Remove the speculative frame.
+          frames_.pop_back();
+        }
         break;
       } else if (in_device_map) {
         // Do not attempt any other unwinding, pc or sp is in a device
diff --git a/libunwindstack/tests/UnwinderTest.cpp b/libunwindstack/tests/UnwinderTest.cpp
index 831d3b5b5..c4b87631c 100644
--- a/libunwindstack/tests/UnwinderTest.cpp
+++ b/libunwindstack/tests/UnwinderTest.cpp
@@ -659,6 +659,54 @@ TEST_F(UnwinderTest, speculative_frame_removed) {
   ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame0", 0));
   ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame1", 1));
 
+  // Fake as if code called a nullptr function.
+  regs_.set_pc(0x20000);
+  regs_.set_sp(0x10000);
+  ElfInterfaceFake::FakePushStepData(StepData(0, 0x10010, false));
+  regs_.FakeSetReturnAddress(0x12);
+  regs_.FakeSetReturnAddressValid(true);
+
+  Unwinder unwinder(64, &maps_, &regs_, process_memory_);
+  unwinder.Unwind();
+  EXPECT_EQ(ERROR_INVALID_MAP, unwinder.LastErrorCode());
+
+  ASSERT_EQ(2U, unwinder.NumFrames());
+
+  auto* frame = &unwinder.frames()[0];
+  EXPECT_EQ(0U, frame->num);
+  EXPECT_EQ(0U, frame->rel_pc);
+  EXPECT_EQ(0x20000U, frame->pc);
+  EXPECT_EQ(0x10000U, frame->sp);
+  EXPECT_EQ("Frame0", frame->function_name);
+  EXPECT_EQ(0U, frame->function_offset);
+  EXPECT_EQ("/system/fake/libunwind.so", frame->map_name);
+  EXPECT_EQ(0U, frame->map_offset);
+  EXPECT_EQ(0x20000U, frame->map_start);
+  EXPECT_EQ(0x22000U, frame->map_end);
+  EXPECT_EQ(0U, frame->map_load_bias);
+  EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags);
+
+  frame = &unwinder.frames()[1];
+  EXPECT_EQ(1U, frame->num);
+  EXPECT_EQ(0U, frame->rel_pc);
+  EXPECT_EQ(0U, frame->pc);
+  EXPECT_EQ(0x10010U, frame->sp);
+  EXPECT_EQ("", frame->function_name);
+  EXPECT_EQ(0U, frame->function_offset);
+  EXPECT_EQ("", frame->map_name);
+  EXPECT_EQ(0U, frame->map_offset);
+  EXPECT_EQ(0U, frame->map_start);
+  EXPECT_EQ(0U, frame->map_end);
+  EXPECT_EQ(0U, frame->map_load_bias);
+  EXPECT_EQ(0, frame->map_flags);
+}
+
+// Verify that a speculative frame is added and left if there are only
+// two frames and the pc is in the middle nowhere.
+TEST_F(UnwinderTest, speculative_frame_not_removed_pc_bad) {
+  ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame0", 0));
+  ElfInterfaceFake::FakePushFunctionData(FunctionData("Frame1", 1));
+
   // Fake as if code called a nullptr function.
   regs_.set_pc(0);
   regs_.set_sp(0x10000);
@@ -669,7 +717,7 @@ TEST_F(UnwinderTest, speculative_frame_removed) {
   unwinder.Unwind();
   EXPECT_EQ(ERROR_NONE, unwinder.LastErrorCode());
 
-  ASSERT_EQ(1U, unwinder.NumFrames());
+  ASSERT_EQ(2U, unwinder.NumFrames());
 
   auto* frame = &unwinder.frames()[0];
   EXPECT_EQ(0U, frame->num);
@@ -684,6 +732,20 @@ TEST_F(UnwinderTest, speculative_frame_removed) {
   EXPECT_EQ(0U, frame->map_end);
   EXPECT_EQ(0U, frame->map_load_bias);
   EXPECT_EQ(0, frame->map_flags);
+
+  frame = &unwinder.frames()[1];
+  EXPECT_EQ(1U, frame->num);
+  EXPECT_EQ(0x200U, frame->rel_pc);
+  EXPECT_EQ(0x1200U, frame->pc);
+  EXPECT_EQ(0x10000U, frame->sp);
+  EXPECT_EQ("Frame0", frame->function_name);
+  EXPECT_EQ(0U, frame->function_offset);
+  EXPECT_EQ("/system/fake/libc.so", frame->map_name);
+  EXPECT_EQ(0U, frame->map_offset);
+  EXPECT_EQ(0x1000U, frame->map_start);
+  EXPECT_EQ(0x8000U, frame->map_end);
+  EXPECT_EQ(0U, frame->map_load_bias);
+  EXPECT_EQ(PROT_READ | PROT_WRITE, frame->map_flags);
 }
 
 // Verify that an unwind stops when a frame is in given suffix.