Make ElfFile not crash on invalid ELF files

Summary:
@rkroll wants to run this on more than just fbcode binaries; there are x86 (not
x86_64) binaries in there and probably a lot of other junk. So, if you call
openNoThrow explicitly, you get a pretty error message in this case.

Test Plan: folly/experimental/symbolizer/test

Reviewed By: lucian@fb.com

Subscribers: rkroll, jhj, lesha, kma

FB internal diff: D1453758

diff --git a/folly/experimental/symbolizer/Elf.cpp b/folly/experimental/symbolizer/Elf.cpp
index 6f525f3fd40780a5ea5439ca65a31b0bdb64bd0a..b4b76e13ed3a6ac97fca6677b4432ce789595b49 100644 (file)
--- a/folly/experimental/symbolizer/Elf.cpp
+++ b/folly/experimental/symbolizer/Elf.cpp
@@ -50,7 +50,11 @@ ElfFile::ElfFile(const char* name, bool readOnly)
 void ElfFile::open(const char* name, bool readOnly) {
   const char* msg = "";
   int r = openNoThrow(name, readOnly, &msg);
-  folly::checkUnixError(r, msg);
+  if (r == kSystemError) {
+    throwSystemError(msg);
+  } else {
+    CHECK_EQ(r, kSuccess) << msg;
+  }
 }
 
 int ElfFile::openNoThrow(const char* name, bool readOnly, const char** msg)
@@ -59,14 +63,14 @@ int ElfFile::openNoThrow(const char* name, bool readOnly, const char** msg)
   fd_ = ::open(name, readOnly ? O_RDONLY : O_RDWR);
   if (fd_ == -1) {
     if (msg) *msg = "open";
-    return -1;
+    return kSystemError;
   }
 
   struct stat st;
   int r = fstat(fd_, &st);
   if (r == -1) {
     if (msg) *msg = "fstat";
-    return -1;
+    return kSystemError;
   }
 
   length_ = st.st_size;
@@ -77,17 +81,21 @@ int ElfFile::openNoThrow(const char* name, bool readOnly, const char** msg)
   file_ = static_cast<char*>(mmap(nullptr, length_, prot, MAP_SHARED, fd_, 0));
   if (file_ == MAP_FAILED) {
     if (msg) *msg = "mmap";
-    return -1;
+    return kSystemError;
+  }
+  if (!init(msg)) {
+    errno = EINVAL;
+    return kInvalidElfFile;
   }
-  init();
-  return 0;
+
+  return kSuccess;
 }
 
 ElfFile::~ElfFile() {
   destroy();
 }
 
-ElfFile::ElfFile(ElfFile&& other)
+ElfFile::ElfFile(ElfFile&& other) noexcept
   : fd_(other.fd_),
     file_(other.file_),
     length_(other.length_),
@@ -125,22 +133,26 @@ void ElfFile::destroy() {
   }
 }
 
-void ElfFile::init() {
+bool ElfFile::init(const char** msg) {
   auto& elfHeader = this->elfHeader();
 
   // Validate ELF magic numbers
-  FOLLY_SAFE_CHECK(elfHeader.e_ident[EI_MAG0] == ELFMAG0 &&
-                   elfHeader.e_ident[EI_MAG1] == ELFMAG1 &&
-                   elfHeader.e_ident[EI_MAG2] == ELFMAG2 &&
-                   elfHeader.e_ident[EI_MAG3] == ELFMAG3,
-                   "invalid ELF magic");
+  if (!(elfHeader.e_ident[EI_MAG0] == ELFMAG0 &&
+        elfHeader.e_ident[EI_MAG1] == ELFMAG1 &&
+        elfHeader.e_ident[EI_MAG2] == ELFMAG2 &&
+        elfHeader.e_ident[EI_MAG3] == ELFMAG3)) {
+    if (msg) *msg = "invalid ELF magic";
+    return false;
+  }
 
   // Validate ELF class (32/64 bits)
 #define EXPECTED_CLASS P1(ELFCLASS, __ELF_NATIVE_CLASS)
 #define P1(a, b) P2(a, b)
 #define P2(a, b) a ## b
-  FOLLY_SAFE_CHECK(elfHeader.e_ident[EI_CLASS] == EXPECTED_CLASS,
-                   "invalid ELF class");
+  if (elfHeader.e_ident[EI_CLASS] != EXPECTED_CLASS) {
+    if (msg) *msg = "invalid ELF class";
+    return false;
+  }
 #undef P1
 #undef P2
 #undef EXPECTED_CLASS
@@ -153,24 +165,38 @@ void ElfFile::init() {
 #else
 # error Unsupported byte order
 #endif
-  FOLLY_SAFE_CHECK(elfHeader.e_ident[EI_DATA] == EXPECTED_ENCODING,
-                   "invalid ELF encoding");
+  if (elfHeader.e_ident[EI_DATA] != EXPECTED_ENCODING) {
+    if (msg) *msg = "invalid ELF encoding";
+    return false;
+  }
 #undef EXPECTED_ENCODING
 
   // Validate ELF version (1)
-  FOLLY_SAFE_CHECK(elfHeader.e_ident[EI_VERSION] == EV_CURRENT &&
-                   elfHeader.e_version == EV_CURRENT,
-                   "invalid ELF version");
+  if (elfHeader.e_ident[EI_VERSION] != EV_CURRENT ||
+      elfHeader.e_version != EV_CURRENT) {
+    if (msg) *msg = "invalid ELF version";
+    return false;
+  }
 
   // We only support executable and shared object files
-  FOLLY_SAFE_CHECK(elfHeader.e_type == ET_EXEC || elfHeader.e_type == ET_DYN,
-                   "invalid ELF file type");
+  if (elfHeader.e_type != ET_EXEC && elfHeader.e_type != ET_DYN) {
+    if (msg) *msg = "invalid ELF file type";
+    return false;
+  }
 
-  FOLLY_SAFE_CHECK(elfHeader.e_phnum != 0, "no program header!");
-  FOLLY_SAFE_CHECK(elfHeader.e_phentsize == sizeof(ElfW(Phdr)),
-                   "invalid program header entry size");
-  FOLLY_SAFE_CHECK(elfHeader.e_shentsize == sizeof(ElfW(Shdr)),
-                   "invalid section header entry size");
+  if (elfHeader.e_phnum == 0) {
+    if (msg) *msg = "no program header!";
+    return false;
+  }
+
+  if (elfHeader.e_phentsize != sizeof(ElfW(Phdr))) {
+    if (msg) *msg = "invalid program header entry size";
+    return false;
+  }
+
+  if (elfHeader.e_shentsize != sizeof(ElfW(Shdr))) {
+    if (msg) *msg = "invalid section header entry size";
+  }
 
   const ElfW(Phdr)* programHeader = &at<ElfW(Phdr)>(elfHeader.e_phoff);
   bool foundBase = false;
@@ -184,7 +210,12 @@ void ElfFile::init() {
     }
   }
 
-  FOLLY_SAFE_CHECK(foundBase, "could not find base address");
+  if (!foundBase) {
+    if (msg) *msg =  "could not find base address";
+    return false;
+  }
+
+  return true;
 }
 
 const ElfW(Shdr)* ElfFile::getSectionByIndex(size_t idx) const {

diff --git a/folly/experimental/symbolizer/Elf.h b/folly/experimental/symbolizer/Elf.h
index f1c4933431f83f77f3bd9e3e3798a151dc3cbbc2..6214a4d566b0bf9bf0a17bd849eb252799123542 100644 (file)
--- a/folly/experimental/symbolizer/Elf.h
+++ b/folly/experimental/symbolizer/Elf.h
@@ -49,8 +49,15 @@ class ElfFile {
   explicit ElfFile(const char* name, bool readOnly=true);
 
   // Open the ELF file.
-  // Returns 0 on success, -1 (and sets errno) on failure and (if msg is not
-  // NULL) sets *msg to a static string indicating what failed.
+  // Returns 0 on success, kSystemError (guaranteed to be -1) (and sets errno)
+  // on IO error, kInvalidElfFile (and sets errno to EINVAL) for an invalid
+  // Elf file. On error, if msg is not NULL, sets *msg to a static string
+  // indicating what failed.
+  enum {
+    kSuccess = 0,
+    kSystemError = -1,
+    kInvalidElfFile = -2,
+  };
   int openNoThrow(const char* name, bool readOnly=true,
                   const char** msg=nullptr) noexcept;
 
@@ -59,7 +66,7 @@ class ElfFile {
 
   ~ElfFile();
 
-  ElfFile(ElfFile&& other);
+  ElfFile(ElfFile&& other) noexcept;
   ElfFile& operator=(ElfFile&& other);
 
   /** Retrieve the ELF header */
@@ -185,7 +192,7 @@ class ElfFile {
   const ElfW(Shdr)* getSectionContainingAddress(ElfW(Addr) addr) const;
 
  private:
-  void init();
+  bool init(const char** msg);
   void destroy();
   ElfFile(const ElfFile&) = delete;
   ElfFile& operator=(const ElfFile&) = delete;

diff --git a/folly/experimental/symbolizer/ElfCache.cpp b/folly/experimental/symbolizer/ElfCache.cpp
index 03fc36d0ba21b41bb9e3316b7374cff7239d8c5d..7aa067bd2a1c0568a73bf746cee2a1ce57314851 100644 (file)
--- a/folly/experimental/symbolizer/ElfCache.cpp
+++ b/folly/experimental/symbolizer/ElfCache.cpp
@@ -46,9 +46,13 @@ std::shared_ptr<ElfFile> SignalSafeElfCache::getFile(StringPiece p) {
   }
 
   auto& f = slots_[n];
-  if (f->openNoThrow(path.data()) == -1) {
+
+  const char* msg = "";
+  int r = f->openNoThrow(path.data(), true, &msg);
+  if (r == ElfFile::kSystemError) {
     return nullptr;
   }
+  FOLLY_SAFE_CHECK(r == ElfFile::kSuccess, msg);
 
   map_[path] = n;
   return f;
@@ -73,9 +77,12 @@ std::shared_ptr<ElfFile> ElfCache::getFile(StringPiece p) {
   auto& path = entry->path;
 
   // No negative caching
-  if (entry->file.openNoThrow(path.c_str()) == -1) {
+  const char* msg = "";
+  int r = entry->file.openNoThrow(path.c_str(), true, &msg);
+  if (r == ElfFile::kSystemError) {
     return nullptr;
   }
+  FOLLY_SAFE_CHECK(r == ElfFile::kSuccess, msg);
 
   if (files_.size() == capacity_) {
     auto& e = lruList_.front();