From 2edcc18349f1b7676d49656d61e1fdd4be07f74f Mon Sep 17 00:00:00 2001
From: David Srbecky <dsrbecky@google.com>
Date: Thu, 13 May 2021 18:40:26 +0100
Subject: Fix symbol look for ART's function end-marker.

Manual zero-sized "end-of-function" marker causes issues,
because it has the same end address the function it marks.

Test: Add test to libunwindstack_unit_test
Change-Id: If9d5970b5200bb254a2a36cadfaa79b5208df1bf
---
 libunwindstack/Symbols.cpp           | 12 ++++++++----
 libunwindstack/tests/SymbolsTest.cpp | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/libunwindstack/Symbols.cpp b/libunwindstack/Symbols.cpp
index 26f893e..499cc31 100644
--- a/libunwindstack/Symbols.cpp
+++ b/libunwindstack/Symbols.cpp
@@ -70,13 +70,15 @@ Symbols::Info* Symbols::BinarySearch(uint64_t addr, Memory* elf_memory, uint64_t
     if (!elf_memory->ReadFully(offset_ + symbol_index * entry_size_, &sym, sizeof(sym))) {
       return nullptr;
     }
-    Info info{.size = static_cast<uint32_t>(sym.st_size), .index = current};
-    it = symbols_.emplace(sym.st_value + sym.st_size, info).first;
+    // There shouldn't be multiple symbols with same end address, but in case there are,
+    // overwrite the cache with the last entry, so that 'sym' and 'info' are consistent.
+    Info& info = symbols_[sym.st_value + sym.st_size];
+    info = {.size = static_cast<uint32_t>(sym.st_size), .index = current};
     if (addr < sym.st_value) {
       last = current;
     } else if (addr < sym.st_value + sym.st_size) {
       *func_offset = addr - sym.st_value;
-      return &it->second;
+      return &info;
     } else {
       first = current + 1;
     }
@@ -104,7 +106,9 @@ void Symbols::BuildRemapTable(Memory* elf_memory) {
       SymType sym;
       memcpy(&sym, &buffer[offset], sizeof(SymType));  // Copy to ensure alignment.
       addrs.push_back(sym.st_value);  // Always insert so it is indexable by symbol index.
-      if (IsFunc(&sym)) {
+      // NB: It is important to filter our zero-sized symbols since otherwise we can get
+      // duplicate end addresses in the table (e.g. if there is custom "end" symbol marker).
+      if (IsFunc(&sym) && sym.st_size != 0) {
         remap_->push_back(symbol_idx);  // Indices of function symbols only.
       }
     }
diff --git a/libunwindstack/tests/SymbolsTest.cpp b/libunwindstack/tests/SymbolsTest.cpp
index bae4e28..89147a2 100644
--- a/libunwindstack/tests/SymbolsTest.cpp
+++ b/libunwindstack/tests/SymbolsTest.cpp
@@ -307,6 +307,36 @@ TYPED_TEST_P(SymbolsTest, symtab_read_cached) {
   ASSERT_EQ(3U, func_offset);
 }
 
+TYPED_TEST_P(SymbolsTest, symtab_end_marker) {
+  Symbols symbols(0x1000, 3 * sizeof(TypeParam), sizeof(TypeParam), 0xa000, 0x1000);
+
+  TypeParam sym;
+  uint64_t offset = 0x1000;
+
+  // Add normal symbol function: Let's say this could be symbol from hand written assembly.
+  this->InitSym(&sym, 0x1000, 0x500, 0x100);
+  this->memory_.SetMemory(offset, &sym, sizeof(sym));
+  offset += sizeof(sym);
+
+  // And zero-sized symbol: A programmer might do that to label the end of assembly method.
+  // This might be a challenge for booking since both symbols end at the same address.
+  this->InitSym(&sym, 0x1500, 0x000, 0x200);
+  this->memory_.SetMemory(offset, &sym, sizeof(sym));
+  offset += sizeof(sym);
+
+  std::string fake_name;
+  fake_name = "entry";
+  this->memory_.SetMemory(0xa100, fake_name.c_str(), fake_name.size() + 1);
+  fake_name = "entry_end";
+  this->memory_.SetMemory(0xa200, fake_name.c_str(), fake_name.size() + 1);
+
+  SharedString name;
+  uint64_t func_offset;
+  ASSERT_TRUE(symbols.GetName<TypeParam>(0x1250, &this->memory_, &name, &func_offset));
+  ASSERT_EQ("entry", name);
+  ASSERT_EQ(0x250U, func_offset);
+}
+
 TYPED_TEST_P(SymbolsTest, get_global) {
   uint64_t start_offset = 0x1000;
   uint64_t str_offset = 0xa000;
@@ -377,7 +407,7 @@ TYPED_TEST_P(SymbolsTest, get_global) {
 
 REGISTER_TYPED_TEST_SUITE_P(SymbolsTest, function_bounds_check, no_symbol, multiple_entries,
                             multiple_entries_nonstandard_size, symtab_value_out_of_bounds,
-                            symtab_read_cached, get_global);
+                            symtab_read_cached, get_global, symtab_end_marker);
 
 typedef ::testing::Types<Elf32_Sym, Elf64_Sym> SymbolsTestTypes;
 INSTANTIATE_TYPED_TEST_SUITE_P(Libunwindstack, SymbolsTest, SymbolsTestTypes);
-- 
cgit v1.2.3