From: Tudor Bosman <tudorb@fb.com>
Date: Sun, 20 Apr 2014 15:02:08 +0000 (-0700)
Subject: SymbolizedFrame::name is already null-terminated
X-Git-Tag: v0.22.0~580
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=5ed60bc7bfdde37f4ae530ebb22e9b074f092414

SymbolizedFrame::name is already null-terminated

Summary:
... as it comes from Elf::getSymbolName, which returns null-terminated C
strings. So there's no need to copy it into a fixed-size buffer (and have a
buffer overflow, ouch).

Test Plan: folly/experimental/symbolizer_test, see what else "arc unit" gets us

Reviewed By: tconerly@fb.com

FB internal diff: D1286348
---

diff --git a/folly/experimental/symbolizer/Symbolizer.cpp b/folly/experimental/symbolizer/Symbolizer.cpp
index ac30db8c..38c925a3 100644
--- a/folly/experimental/symbolizer/Symbolizer.cpp
+++ b/folly/experimental/symbolizer/Symbolizer.cpp
@@ -172,10 +172,7 @@ void SymbolizedFrame::set(const std::shared_ptr<ElfFile>& file,
   }
 
   file_ = file;
-  auto name = file->getSymbolName(sym);
-  if (name) {
-    this->name = name;
-  }
+  name = file->getSymbolName(sym);
 
   Dwarf(file.get()).findAddress(address, location);
 }
@@ -306,25 +303,18 @@ void SymbolizePrinter::print(uintptr_t address, const SymbolizedFrame& frame) {
                          sizeof(padBuf) - 1 - (16 - 2 * sizeof(uintptr_t)));
 
   color(kFunctionColor);
-  char mangledBuf[1024];
   if (!frame.found) {
     doPrint(" (not found)");
     return;
   }
 
-  if (frame.name.empty()) {
+  if (!frame.name || frame.name[0] == '\0') {
     doPrint(" (unknown)");
-  } else if (frame.name.size() >= sizeof(mangledBuf)) {
-    doPrint(" ");
-    doPrint(frame.name);
   } else {
-    memcpy(mangledBuf, frame.name.data(), frame.name.size());
-    mangledBuf[frame.name.size()] = '\0';
-
     char demangledBuf[1024];
-    demangle(mangledBuf, demangledBuf, sizeof(demangledBuf));
+    demangle(frame.name, demangledBuf, sizeof(demangledBuf));
     doPrint(" ");
-    doPrint(demangledBuf);
+    doPrint(demangledBuf[0] == '\0' ? frame.name : demangledBuf);
   }
 
   if (!(options_ & NO_FILE_AND_LINE)) {
@@ -392,14 +382,10 @@ void SymbolizePrinter::println(uintptr_t address,
 
 void SymbolizePrinter::printTerse(uintptr_t address,
                                   const SymbolizedFrame& frame) {
-  if (frame.found) {
-    char mangledBuf[1024];
-    memcpy(mangledBuf, frame.name.data(), frame.name.size());
-    mangledBuf[frame.name.size()] = '\0';
-
+  if (frame.found && frame.name && frame.name[0] != '\0') {
     char demangledBuf[1024] = {0};
-    demangle(mangledBuf, demangledBuf, sizeof(demangledBuf));
-    doPrint(strlen(demangledBuf) == 0 ? "(unknown)" : demangledBuf);
+    demangle(frame.name, demangledBuf, sizeof(demangledBuf));
+    doPrint(demangledBuf[0] == '\0' ? frame.name : demangledBuf);
   } else {
     // Can't use sprintf, not async-signal-safe
     static_assert(sizeof(uintptr_t) <= 8, "huge uintptr_t?");
diff --git a/folly/experimental/symbolizer/Symbolizer.h b/folly/experimental/symbolizer/Symbolizer.h
index 00006141..88619a99 100644
--- a/folly/experimental/symbolizer/Symbolizer.h
+++ b/folly/experimental/symbolizer/Symbolizer.h
@@ -38,21 +38,21 @@ class Symbolizer;
  * Frame information: symbol name and location.
  */
 struct SymbolizedFrame {
-  SymbolizedFrame() : found(false) { }
+  SymbolizedFrame() : found(false), name(nullptr) { }
 
   void set(const std::shared_ptr<ElfFile>& file, uintptr_t address);
   void clear() { *this = SymbolizedFrame(); }
 
   bool isSignalFrame;
   bool found;
-  StringPiece name;
+  const char* name;
   Dwarf::LocationInfo location;
 
   /**
    * Demangle the name and return it. Not async-signal-safe; allocates memory.
    */
   fbstring demangledName() const {
-    return demangle(name.fbstr().c_str());
+    return name ? demangle(name) : fbstring();
   }
  private:
   std::shared_ptr<ElfFile> file_;
diff --git a/folly/experimental/symbolizer/test/SymbolizerTest.cpp b/folly/experimental/symbolizer/test/SymbolizerTest.cpp
index d283dc82..73862553 100644
--- a/folly/experimental/symbolizer/test/SymbolizerTest.cpp
+++ b/folly/experimental/symbolizer/test/SymbolizerTest.cpp
@@ -32,8 +32,7 @@ TEST(Symbolizer, Single) {
   Symbolizer symbolizer;
   SymbolizedFrame a;
   ASSERT_TRUE(symbolizer.symbolize(reinterpret_cast<uintptr_t>(foo), a));
-  EXPECT_EQ("folly::symbolizer::test::foo()",
-            demangle(a.name.str().c_str()));
+  EXPECT_EQ("folly::symbolizer::test::foo()", a.demangledName());
 
   auto path = a.location.file.toString();
   folly::StringPiece basename(path);
@@ -80,14 +79,12 @@ void ElfCacheTest::SetUp() {
 void runElfCacheTest(Symbolizer& symbolizer) {
   FrameArray<100> frames = goldenFrames;
   for (size_t i = 0; i < frames.frameCount; ++i) {
-    auto& f = frames.frames[i];
-    f.found = false;
-    f.name.clear();
+    frames.frames[i].clear();
   }
   symbolizer.symbolize(frames);
   ASSERT_LE(4, frames.frameCount);
   for (size_t i = 1; i < 4; ++i) {
-    EXPECT_EQ(goldenFrames.frames[i].name, frames.frames[i].name);
+    EXPECT_STREQ(goldenFrames.frames[i].name, frames.frames[i].name);
   }
 }