Fix a crash in running llvm-objdump -t with an invalid Mach-O file already

in the test suite. While this is not really an interesting tool and option to run
on a Mach-O file to show the symbol table in a generic libObject format
it shouldn’t crash.

The reason for the crash was in MachOObjectFile::getSymbolType() when it was
calling MachOObjectFile::getSymbolSection() without checking its return value
for the error case.

What makes this fix require a fair bit of diffs is that the method getSymbolType() is
in the class ObjectFile defined without an ErrorOr<> so I needed to add that all
the sub classes.  And all of the uses needed to be updated and the return value
needed to be checked for the error case.

The MachOObjectFile version of getSymbolType() “can” get an error in trying to
come up with the libObject’s internal SymbolRef::Type when the Mach-O symbol
symbol type is an N_SECT type because the code is trying to select from the
SymbolRef::ST_Data or SymbolRef::ST_Function values for the SymbolRef::Type.
And it needs the Mach-O section to use isData() and isBSS to determine if
it will return SymbolRef::ST_Data.

One other possible fix I considered is to simply return SymbolRef::ST_Other
when MachOObjectFile::getSymbolSection() returned an error.  But since in
the past when I did such changes that “ate an error in the libObject code” I
was asked instead to push the error out of the libObject code I chose not
to implement the fix this way.

As currently written both the COFF and ELF versions of getSymbolType()
can’t get an error.  But if isReservedSectionNumber() wanted to check for
the two known negative values rather than allowing all negative values or
the code wanted to add the same check as in getSymbolAddress() to use
getSection() and check for the error then these versions of getSymbolType()
could return errors.

At the end of the day the error printed now is the generic “Invalid data was
encountered while parsing the file” for object_error::parse_failed.  In the
future when we thread Lang’s new TypedError for recoverable error handling
though libObject this will improve.  And where the added // Diagnostic(…
comment is, it would be changed to produce and error message
like “bad section index (42) for symbol at index 8” for this case.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@264187 91177308-0d34-0410-b5e6-96231b3b80d8

15 files changed, 75 insertions, 24 deletions

diff --git a/include/llvm/Object/COFF.h b/include/llvm/Object/COFF.h
index 4aad87c984b..5ae58051b78 100644
--- a/include/llvm/Object/COFF.h
+++ b/include/llvm/Object/COFF.h
@@ -684,7 +684,7 @@ protected:
   uint64_t getSymbolValueImpl(DataRefImpl Symb) const override;
   uint64_t getCommonSymbolSizeImpl(DataRefImpl Symb) const override;
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
-  SymbolRef::Type getSymbolType(DataRefImpl Symb) const override;
+  ErrorOr<SymbolRef::Type> getSymbolType(DataRefImpl Symb) const override;
   ErrorOr<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
   void moveSectionNext(DataRefImpl &Sec) const override;
   std::error_code getSectionName(DataRefImpl Sec,
diff --git a/include/llvm/Object/ELFObjectFile.h b/include/llvm/Object/ELFObjectFile.h
index b01fa1da4b3..efff38f11c4 100644
--- a/include/llvm/Object/ELFObjectFile.h
+++ b/include/llvm/Object/ELFObjectFile.h
@@ -205,7 +205,7 @@ protected:
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
   uint8_t getSymbolOther(DataRefImpl Symb) const override;
   uint8_t getSymbolELFType(DataRefImpl Symb) const override;
-  SymbolRef::Type getSymbolType(DataRefImpl Symb) const override;
+  ErrorOr<SymbolRef::Type> getSymbolType(DataRefImpl Symb) const override;
   ErrorOr<section_iterator> getSymbolSection(const Elf_Sym *Symb,
                                              const Elf_Shdr *SymTab) const;
   ErrorOr<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
@@ -445,7 +445,8 @@ uint8_t ELFObjectFile<ELFT>::getSymbolELFType(DataRefImpl Symb) const {
 }
 
 template <class ELFT>
-SymbolRef::Type ELFObjectFile<ELFT>::getSymbolType(DataRefImpl Symb) const {
+ErrorOr<SymbolRef::Type>
+ELFObjectFile<ELFT>::getSymbolType(DataRefImpl Symb) const {
   const Elf_Sym *ESym = getSymbol(Symb);
 
   switch (ESym->getType()) {
diff --git a/include/llvm/Object/MachO.h b/include/llvm/Object/MachO.h
index 252d47d3c4c..fc88b18cb91 100644
--- a/include/llvm/Object/MachO.h
+++ b/include/llvm/Object/MachO.h
@@ -208,7 +208,7 @@ public:
   ErrorOr<uint64_t> getSymbolAddress(DataRefImpl Symb) const override;
   uint32_t getSymbolAlignment(DataRefImpl Symb) const override;
   uint64_t getCommonSymbolSizeImpl(DataRefImpl Symb) const override;
-  SymbolRef::Type getSymbolType(DataRefImpl Symb) const override;
+  ErrorOr<SymbolRef::Type> getSymbolType(DataRefImpl Symb) const override;
   uint32_t getSymbolFlags(DataRefImpl Symb) const override;
   ErrorOr<section_iterator> getSymbolSection(DataRefImpl Symb) const override;
   unsigned getSymbolSectionID(SymbolRef Symb) const;
diff --git a/include/llvm/Object/ObjectFile.h b/include/llvm/Object/ObjectFile.h
index 6fe73855dca..6aeeb87be1d 100644
--- a/include/llvm/Object/ObjectFile.h
+++ b/include/llvm/Object/ObjectFile.h
@@ -143,7 +143,7 @@ public:
   /// @brief Get the alignment of this symbol as the actual value (not log 2).
   uint32_t getAlignment() const;
   uint64_t getCommonSize() const;
-  SymbolRef::Type getType() const;
+  ErrorOr<SymbolRef::Type> getType() const;
 
   /// @brief Get section this symbol is defined in reference to. Result is
   /// end_sections() if it is undefined or is an absolute symbol.
@@ -201,7 +201,7 @@ protected:
   virtual uint64_t getSymbolValueImpl(DataRefImpl Symb) const = 0;
   virtual uint32_t getSymbolAlignment(DataRefImpl Symb) const;
   virtual uint64_t getCommonSymbolSizeImpl(DataRefImpl Symb) const = 0;
-  virtual SymbolRef::Type getSymbolType(DataRefImpl Symb) const = 0;
+  virtual ErrorOr<SymbolRef::Type> getSymbolType(DataRefImpl Symb) const = 0;
   virtual ErrorOr<section_iterator>
   getSymbolSection(DataRefImpl Symb) const = 0;
 
@@ -329,7 +329,7 @@ inline ErrorOr<section_iterator> SymbolRef::getSection() const {
   return getObject()->getSymbolSection(getRawDataRefImpl());
 }
 
-inline SymbolRef::Type SymbolRef::getType() const {
+inline ErrorOr<SymbolRef::Type> SymbolRef::getType() const {
   return getObject()->getSymbolType(getRawDataRefImpl());
 }
 
diff --git a/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp b/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp
index e3146245984..59efdeff763 100644
--- a/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp
+++ b/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp
@@ -119,7 +119,10 @@ std::error_code SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
                                                   uint64_t SymbolSize,
                                                   DataExtractor *OpdExtractor,
                                                   uint64_t OpdAddress) {
-  SymbolRef::Type SymbolType = Symbol.getType();
+  ErrorOr<SymbolRef::Type> SymbolTypeOrErr = Symbol.getType();
+  if (auto EC = SymbolTypeOrErr.getError())
+    return EC;
+  SymbolRef::Type SymbolType = *SymbolTypeOrErr;
   if (SymbolType != SymbolRef::ST_Function && SymbolType != SymbolRef::ST_Data)
     return std::error_code();
   ErrorOr<uint64_t> SymbolAddressOrErr = Symbol.getAddress();
diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
index d16b2db24e1..f9c2b3f333e 100644
--- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
+++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
@@ -169,7 +169,9 @@ RuntimeDyldImpl::loadObjectImpl(const object::ObjectFile &Obj) {
     if (Flags & SymbolRef::SF_Common)
       CommonSymbols.push_back(*I);
     else {
-      object::SymbolRef::Type SymType = I->getType();
+      ErrorOr<object::SymbolRef::Type> SymTypeOrErr = I->getType();
+      Check(SymTypeOrErr.getError());
+      object::SymbolRef::Type SymType = *SymTypeOrErr;
 
       // Get symbol name.
       ErrorOr<StringRef> NameOrErr = I->getName();
diff --git a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
index 539e48baafd..1668867cd4d 100644
--- a/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
+++ b/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
@@ -1190,7 +1190,10 @@ relocation_iterator RuntimeDyldELF::processRelocationRef(
   RTDyldSymbolTable::const_iterator gsi = GlobalSymbolTable.end();
   if (Symbol != Obj.symbol_end()) {
     gsi = GlobalSymbolTable.find(TargetName.data());
-    SymType = Symbol->getType();
+    ErrorOr<SymbolRef::Type> SymTypeOrErr = Symbol->getType();
+    if (std::error_code EC = SymTypeOrErr.getError())
+      report_fatal_error(EC.message());
+    SymType = *SymTypeOrErr;
   }
   if (gsi != GlobalSymbolTable.end()) {
     const auto &SymInfo = gsi->second;
diff --git a/lib/Object/COFFObjectFile.cpp b/lib/Object/COFFObjectFile.cpp
index 25145471382..3d144089a20 100644
--- a/lib/Object/COFFObjectFile.cpp
+++ b/lib/Object/COFFObjectFile.cpp
@@ -179,7 +179,7 @@ ErrorOr<uint64_t> COFFObjectFile::getSymbolAddress(DataRefImpl Ref) const {
   return Result;
 }
 
-SymbolRef::Type COFFObjectFile::getSymbolType(DataRefImpl Ref) const {
+ErrorOr<SymbolRef::Type> COFFObjectFile::getSymbolType(DataRefImpl Ref) const {
   COFFSymbolRef Symb = getCOFFSymbol(Ref);
   int32_t SectionNumber = Symb.getSectionNumber();
 
diff --git a/lib/Object/MachOObjectFile.cpp b/lib/Object/MachOObjectFile.cpp
index 3a892d21db3..340faa44a31 100644
--- a/lib/Object/MachOObjectFile.cpp
+++ b/lib/Object/MachOObjectFile.cpp
@@ -443,7 +443,8 @@ uint64_t MachOObjectFile::getCommonSymbolSizeImpl(DataRefImpl DRI) const {
   return getNValue(DRI);
 }
 
-SymbolRef::Type MachOObjectFile::getSymbolType(DataRefImpl Symb) const {
+ErrorOr<SymbolRef::Type>
+MachOObjectFile::getSymbolType(DataRefImpl Symb) const {
   MachO::nlist_base Entry = getSymbolTableEntryBase(this, Symb);
   uint8_t n_type = Entry.n_type;
 
@@ -455,7 +456,10 @@ SymbolRef::Type MachOObjectFile::getSymbolType(DataRefImpl Symb) const {
     case MachO::N_UNDF :
       return SymbolRef::ST_Unknown;
     case MachO::N_SECT :
-      section_iterator Sec = *getSymbolSection(Symb);
+      ErrorOr<section_iterator> SecOrError = getSymbolSection(Symb);
+      if (!SecOrError)
+        return SecOrError.getError();
+      section_iterator Sec = *SecOrError;
       if (Sec->isData() || Sec->isBSS())
         return SymbolRef::ST_Data;
       return SymbolRef::ST_Function;
@@ -511,8 +515,11 @@ MachOObjectFile::getSymbolSection(DataRefImpl Symb) const {
     return section_end();
   DataRefImpl DRI;
   DRI.d.a = index - 1;
-  if (DRI.d.a >= Sections.size())
+  if (DRI.d.a >= Sections.size()){
+    // Diagnostic("bad section index (" + index + ") for symbol at index " +
+    //  SymbolIndex);
     return object_error::parse_failed;
+  }
   return section_iterator(SectionRef(DRI, this));
 }
 
diff --git a/test/Object/macho-invalid.test b/test/Object/macho-invalid.test
index cbd378a0332..9117ebec890 100644
--- a/test/Object/macho-invalid.test
+++ b/test/Object/macho-invalid.test
@@ -54,6 +54,9 @@ INVALID-SECTION-IDX-SYMBOL-SEC-m: 0000000100000000 (?,?) [referenced dynamically
 RUN: llvm-nm -pax %p/Inputs/macho-invalid-section-index-getSectionRawName 2>&1 \
 RUN:      | FileCheck -check-prefix INVALID-SECTION-IDX-SYMBOL-SEC-pax %s
 INVALID-SECTION-IDX-SYMBOL-SEC-pax: 0000000100000000 0f 42 0010 00000065 __mh_execute_header
+RUN: not llvm-objdump -t %p/Inputs/macho-invalid-section-index-getSectionRawName 2>&1 \
+RUN:      | FileCheck -check-prefix INVALID-SECTION-IDX-SYMBOL-SEC-objdump %s
+INVALID-SECTION-IDX-SYMBOL-SEC-objdump: Invalid data was encountered while parsing the file.
 
 RUN: not llvm-objdump -private-headers %p/Inputs/macho-invalid-header 2>&1 | FileCheck -check-prefix INVALID-HEADER %s
 INVALID-HEADER: The file was not recognized as a valid object file.
diff --git a/tools/dsymutil/MachODebugMapParser.cpp b/tools/dsymutil/MachODebugMapParser.cpp
index 02c3ab07f6a..29529942cb8 100644
--- a/tools/dsymutil/MachODebugMapParser.cpp
+++ b/tools/dsymutil/MachODebugMapParser.cpp
@@ -437,7 +437,10 @@ void MachODebugMapParser::loadMainBinarySymbols(
   section_iterator Section = MainBinary.section_end();
   MainBinarySymbolAddresses.clear();
   for (const auto &Sym : MainBinary.symbols()) {
-    SymbolRef::Type Type = Sym.getType();
+    ErrorOr<SymbolRef::Type> TypeOrErr = Sym.getType();
+    if (!TypeOrErr)
+      continue;
+    SymbolRef::Type Type = *TypeOrErr;
     // Skip undefined and STAB entries.
     if ((Type & SymbolRef::ST_Debug) || (Type & SymbolRef::ST_Unknown))
       continue;
diff --git a/tools/llvm-objdump/MachODump.cpp b/tools/llvm-objdump/MachODump.cpp
index 318d8374731..428b737cfec 100644
--- a/tools/llvm-objdump/MachODump.cpp
+++ b/tools/llvm-objdump/MachODump.cpp
@@ -172,8 +172,16 @@ static const Target *GetTarget(const MachOObjectFile *MachOObj,
 
 struct SymbolSorter {
   bool operator()(const SymbolRef &A, const SymbolRef &B) {
-    uint64_t AAddr = (A.getType() != SymbolRef::ST_Function) ? 0 : A.getValue();
-    uint64_t BAddr = (B.getType() != SymbolRef::ST_Function) ? 0 : B.getValue();
+    ErrorOr<SymbolRef::Type> ATypeOrErr = A.getType();
+    if (std::error_code EC = ATypeOrErr.getError())
+        report_fatal_error(EC.message());
+    SymbolRef::Type AType = *ATypeOrErr;
+    ErrorOr<SymbolRef::Type> BTypeOrErr = B.getType();
+    if (std::error_code EC = BTypeOrErr.getError())
+        report_fatal_error(EC.message());
+    SymbolRef::Type BType = *ATypeOrErr;
+    uint64_t AAddr = (AType != SymbolRef::ST_Function) ? 0 : A.getValue();
+    uint64_t BAddr = (BType != SymbolRef::ST_Function) ? 0 : B.getValue();
     return AAddr < BAddr;
   }
 };
@@ -573,7 +581,10 @@ static void CreateSymbolAddressMap(MachOObjectFile *O,
                                    SymbolAddressMap *AddrMap) {
   // Create a map of symbol addresses to symbol names.
   for (const SymbolRef &Symbol : O->symbols()) {
-    SymbolRef::Type ST = Symbol.getType();
+    ErrorOr<SymbolRef::Type> STOrErr = Symbol.getType();
+    if (std::error_code EC = STOrErr.getError())
+        report_fatal_error(EC.message());
+    SymbolRef::Type ST = *STOrErr;
     if (ST == SymbolRef::ST_Function || ST == SymbolRef::ST_Data ||
         ST == SymbolRef::ST_Other) {
       uint64_t Address = Symbol.getValue();
@@ -6083,7 +6094,10 @@ static void DisassembleMachO(StringRef Filename, MachOObjectFile *MachOOF,
     SymbolAddressMap AddrMap;
     bool DisSymNameFound = false;
     for (const SymbolRef &Symbol : MachOOF->symbols()) {
-      SymbolRef::Type ST = Symbol.getType();
+      ErrorOr<SymbolRef::Type> STOrErr = Symbol.getType();
+      if (std::error_code EC = STOrErr.getError())
+          report_fatal_error(EC.message());
+      SymbolRef::Type ST = *STOrErr;
       if (ST == SymbolRef::ST_Function || ST == SymbolRef::ST_Data ||
           ST == SymbolRef::ST_Other) {
         uint64_t Address = Symbol.getValue();
@@ -6134,7 +6148,10 @@ static void DisassembleMachO(StringRef Filename, MachOObjectFile *MachOOF,
         report_fatal_error(EC.message());
       StringRef SymName = *SymNameOrErr;
 
-      SymbolRef::Type ST = Symbols[SymIdx].getType();
+      ErrorOr<SymbolRef::Type> STOrErr = Symbols[SymIdx].getType();
+      if (std::error_code EC = STOrErr.getError())
+          report_fatal_error(EC.message());
+      SymbolRef::Type ST = *STOrErr;
       if (ST != SymbolRef::ST_Function && ST != SymbolRef::ST_Data)
         continue;
 
@@ -6158,7 +6175,10 @@ static void DisassembleMachO(StringRef Filename, MachOObjectFile *MachOOF,
       uint64_t NextSym = 0;
       uint64_t NextSymIdx = SymIdx + 1;
       while (Symbols.size() > NextSymIdx) {
-        SymbolRef::Type NextSymType = Symbols[NextSymIdx].getType();
+        ErrorOr<SymbolRef::Type> STOrErr = Symbols[NextSymIdx].getType();
+        if (std::error_code EC = STOrErr.getError())
+            report_fatal_error(EC.message());
+        SymbolRef::Type NextSymType = *STOrErr;
         if (NextSymType == SymbolRef::ST_Function) {
           containsNextSym =
               Sections[SectIdx].containsSymbol(Symbols[NextSymIdx]);
diff --git a/tools/llvm-objdump/llvm-objdump.cpp b/tools/llvm-objdump/llvm-objdump.cpp
index f436183f065..28d667a486e 100644
--- a/tools/llvm-objdump/llvm-objdump.cpp
+++ b/tools/llvm-objdump/llvm-objdump.cpp
@@ -1293,7 +1293,9 @@ void llvm::PrintSymbolTable(const ObjectFile *o) {
     ErrorOr<uint64_t> AddressOrError = Symbol.getAddress();
     error(AddressOrError.getError());
     uint64_t Address = *AddressOrError;
-    SymbolRef::Type Type = Symbol.getType();
+    ErrorOr<SymbolRef::Type> TypeOrError = Symbol.getType();
+    error(TypeOrError.getError());
+    SymbolRef::Type Type = *TypeOrError;
     uint32_t Flags = Symbol.getFlags();
     ErrorOr<section_iterator> SectionOrErr = Symbol.getSection();
     error(SectionOrErr.getError());
diff --git a/tools/llvm-readobj/ARMWinEHPrinter.cpp b/tools/llvm-readobj/ARMWinEHPrinter.cpp
index 76387c7c7c9..0f74a998707 100644
--- a/tools/llvm-readobj/ARMWinEHPrinter.cpp
+++ b/tools/llvm-readobj/ARMWinEHPrinter.cpp
@@ -198,7 +198,10 @@ Decoder::getSectionContaining(const COFFObjectFile &COFF, uint64_t VA) {
 ErrorOr<object::SymbolRef> Decoder::getSymbol(const COFFObjectFile &COFF,
                                               uint64_t VA, bool FunctionOnly) {
   for (const auto &Symbol : COFF.symbols()) {
-    if (FunctionOnly && Symbol.getType() != SymbolRef::ST_Function)
+    ErrorOr<SymbolRef::Type> Type = Symbol.getType();
+    if (std::error_code EC = Type.getError())
+      return EC;
+    if (FunctionOnly && *Type != SymbolRef::ST_Function)
       continue;
 
     ErrorOr<uint64_t> Address = Symbol.getAddress();
diff --git a/tools/llvm-rtdyld/llvm-rtdyld.cpp b/tools/llvm-rtdyld/llvm-rtdyld.cpp
index 81b9c7dc502..3b5c1821f1b 100644
--- a/tools/llvm-rtdyld/llvm-rtdyld.cpp
+++ b/tools/llvm-rtdyld/llvm-rtdyld.cpp
@@ -330,7 +330,11 @@ static int printLineInfoForInput(bool LoadObjects, bool UseDebugObj) {
     // Use symbol info to iterate functions in the object.
     for (const auto &P : SymAddr) {
       object::SymbolRef Sym = P.first;
-      if (Sym.getType() == object::SymbolRef::ST_Function) {
+      ErrorOr<SymbolRef::Type> TypeOrErr = Sym.getType();
+      if (!TypeOrErr)
+        continue;
+      SymbolRef::Type Type = *TypeOrErr;
+      if (Type == object::SymbolRef::ST_Function) {
         ErrorOr<StringRef> Name = Sym.getName();
         if (!Name)
           continue;