Commit 5ed60bc7 authored by Tudor Bosman's avatar Tudor Bosman Committed by Dave Watson

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
parent 2c73945c
...@@ -172,10 +172,7 @@ void SymbolizedFrame::set(const std::shared_ptr<ElfFile>& file, ...@@ -172,10 +172,7 @@ void SymbolizedFrame::set(const std::shared_ptr<ElfFile>& file,
} }
file_ = file; file_ = file;
auto name = file->getSymbolName(sym); name = file->getSymbolName(sym);
if (name) {
this->name = name;
}
Dwarf(file.get()).findAddress(address, location); Dwarf(file.get()).findAddress(address, location);
} }
...@@ -306,25 +303,18 @@ void SymbolizePrinter::print(uintptr_t address, const SymbolizedFrame& frame) { ...@@ -306,25 +303,18 @@ void SymbolizePrinter::print(uintptr_t address, const SymbolizedFrame& frame) {
sizeof(padBuf) - 1 - (16 - 2 * sizeof(uintptr_t))); sizeof(padBuf) - 1 - (16 - 2 * sizeof(uintptr_t)));
color(kFunctionColor); color(kFunctionColor);
char mangledBuf[1024];
if (!frame.found) { if (!frame.found) {
doPrint(" (not found)"); doPrint(" (not found)");
return; return;
} }
if (frame.name.empty()) { if (!frame.name || frame.name[0] == '\0') {
doPrint(" (unknown)"); doPrint(" (unknown)");
} else if (frame.name.size() >= sizeof(mangledBuf)) {
doPrint(" ");
doPrint(frame.name);
} else { } else {
memcpy(mangledBuf, frame.name.data(), frame.name.size());
mangledBuf[frame.name.size()] = '\0';
char demangledBuf[1024]; char demangledBuf[1024];
demangle(mangledBuf, demangledBuf, sizeof(demangledBuf)); demangle(frame.name, demangledBuf, sizeof(demangledBuf));
doPrint(" "); doPrint(" ");
doPrint(demangledBuf); doPrint(demangledBuf[0] == '\0' ? frame.name : demangledBuf);
} }
if (!(options_ & NO_FILE_AND_LINE)) { if (!(options_ & NO_FILE_AND_LINE)) {
...@@ -392,14 +382,10 @@ void SymbolizePrinter::println(uintptr_t address, ...@@ -392,14 +382,10 @@ void SymbolizePrinter::println(uintptr_t address,
void SymbolizePrinter::printTerse(uintptr_t address, void SymbolizePrinter::printTerse(uintptr_t address,
const SymbolizedFrame& frame) { const SymbolizedFrame& frame) {
if (frame.found) { if (frame.found && frame.name && frame.name[0] != '\0') {
char mangledBuf[1024];
memcpy(mangledBuf, frame.name.data(), frame.name.size());
mangledBuf[frame.name.size()] = '\0';
char demangledBuf[1024] = {0}; char demangledBuf[1024] = {0};
demangle(mangledBuf, demangledBuf, sizeof(demangledBuf)); demangle(frame.name, demangledBuf, sizeof(demangledBuf));
doPrint(strlen(demangledBuf) == 0 ? "(unknown)" : demangledBuf); doPrint(demangledBuf[0] == '\0' ? frame.name : demangledBuf);
} else { } else {
// Can't use sprintf, not async-signal-safe // Can't use sprintf, not async-signal-safe
static_assert(sizeof(uintptr_t) <= 8, "huge uintptr_t?"); static_assert(sizeof(uintptr_t) <= 8, "huge uintptr_t?");
......
...@@ -38,21 +38,21 @@ class Symbolizer; ...@@ -38,21 +38,21 @@ class Symbolizer;
* Frame information: symbol name and location. * Frame information: symbol name and location.
*/ */
struct SymbolizedFrame { struct SymbolizedFrame {
SymbolizedFrame() : found(false) { } SymbolizedFrame() : found(false), name(nullptr) { }
void set(const std::shared_ptr<ElfFile>& file, uintptr_t address); void set(const std::shared_ptr<ElfFile>& file, uintptr_t address);
void clear() { *this = SymbolizedFrame(); } void clear() { *this = SymbolizedFrame(); }
bool isSignalFrame; bool isSignalFrame;
bool found; bool found;
StringPiece name; const char* name;
Dwarf::LocationInfo location; Dwarf::LocationInfo location;
/** /**
* Demangle the name and return it. Not async-signal-safe; allocates memory. * Demangle the name and return it. Not async-signal-safe; allocates memory.
*/ */
fbstring demangledName() const { fbstring demangledName() const {
return demangle(name.fbstr().c_str()); return name ? demangle(name) : fbstring();
} }
private: private:
std::shared_ptr<ElfFile> file_; std::shared_ptr<ElfFile> file_;
......
...@@ -32,8 +32,7 @@ TEST(Symbolizer, Single) { ...@@ -32,8 +32,7 @@ TEST(Symbolizer, Single) {
Symbolizer symbolizer; Symbolizer symbolizer;
SymbolizedFrame a; SymbolizedFrame a;
ASSERT_TRUE(symbolizer.symbolize(reinterpret_cast<uintptr_t>(foo), a)); ASSERT_TRUE(symbolizer.symbolize(reinterpret_cast<uintptr_t>(foo), a));
EXPECT_EQ("folly::symbolizer::test::foo()", EXPECT_EQ("folly::symbolizer::test::foo()", a.demangledName());
demangle(a.name.str().c_str()));
auto path = a.location.file.toString(); auto path = a.location.file.toString();
folly::StringPiece basename(path); folly::StringPiece basename(path);
...@@ -80,14 +79,12 @@ void ElfCacheTest::SetUp() { ...@@ -80,14 +79,12 @@ void ElfCacheTest::SetUp() {
void runElfCacheTest(Symbolizer& symbolizer) { void runElfCacheTest(Symbolizer& symbolizer) {
FrameArray<100> frames = goldenFrames; FrameArray<100> frames = goldenFrames;
for (size_t i = 0; i < frames.frameCount; ++i) { for (size_t i = 0; i < frames.frameCount; ++i) {
auto& f = frames.frames[i]; frames.frames[i].clear();
f.found = false;
f.name.clear();
} }
symbolizer.symbolize(frames); symbolizer.symbolize(frames);
ASSERT_LE(4, frames.frameCount); ASSERT_LE(4, frames.frameCount);
for (size_t i = 1; i < 4; ++i) { 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);
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment