Report fatal errors instead of segfaulting/asserting on a few invalid accesses while reading MachO files.

[filcab]

Summary

Shift an older “invalid file” test to get a consistent naming for these
tests.

[majnemer]

lib/Object/MachOObjectFile.cpp

302

Maybe "Segment load command size is too small."?

[• rafael]

Optional bonus (can be another patch): include more information in the errors: Section index is XXX, but there are only YYY sections.

lib/Object/MachOObjectFile.cpp

45

There are no tests for this one. Please add one or convert this to an assert.

60

Nit. maybe avoid this bit of duplicated with a small helper template function that takes a MachO::segment_command_64 or a MachO::segment_command?

[filcab]

Rephrase error pointed out by majnemer
Added a test requested by Rafael

Rafael: Extracting that part of the function yields code that's a bit too convoluted. Check which version you prefer (or suggest other changes).

[• rafael]

lib/Object/MachOObjectFile.cpp

55

Maybe pass a "const SegmentCmd&"?

2233

Missing test.

2243

Missing test.

test/Object/macho-invalid.test

34

There are 4 errors that would match " Invalid section index": Please make sure they are all covered.

[filcab]

Added some missing tests

[belleyb]

Hi Everyone,

My understanding is that the ELF writer is also used by the MCJIT engine when filling an object buffer with jitted code. In that context, I am wondering if the call to llvm::report_fatal_error() is the appropriate way to report the error. It has the effect of killing the entire host application simply because one LLVM module couldn¹t be jitted.

Of course, fatal errors and assertions should never occur! ;-) And, if they never occur, then there isn¹t any issue. Moreover, it is much better to exit the application after a proper error message has been emitted, then crashing on a segfault! So, this patch is definitely a step in the right direction.

On the other hand, report_fatal_error() has the unfortunate consequence of killing our application before our users have any chance of saving their work.

Thus, I am wondering if there would be a way to indicate to the MCJIT engine that an internal error occurred while jitting a particular LLVM module. The compilation of that module would abort, but the application would not exit.

Is that a legitimate concern ? Comments ?

Cheers,
Benoit

Benoit Belley
Sr Principal Developer
M&E-Product Development Group
Autodesk, Inc.
www.autodesk.com http://www.autodesk.com/

[majnemer]

In D6945#109149, @belleyb wrote:

Hi Everyone,

My understanding is that the ELF writer is also used by the MCJIT engine when filling an object buffer with jitted code. In that context, I am wondering if the call to llvm::report_fatal_error() is the appropriate way to report the error. It has the effect of killing the entire host application simply because one LLVM module couldn¹t be jitted.

Of course, fatal errors and assertions should never occur! ;-) And, if they never occur, then there isn¹t any issue. Moreover, it is much better to exit the application after a proper error message has been emitted, then crashing on a segfault! So, this patch is definitely a step in the right direction.

On the other hand, report_fatal_error() has the unfortunate consequence of killing our application before our users have any chance of saving their work.

Thus, I am wondering if there would be a way to indicate to the MCJIT engine that an internal error occurred while jitting a particular LLVM module. The compilation of that module would abort, but the application would not exit.

Is that a legitimate concern ? Comments ?

Cheers,
Benoit

Benoit Belley
Sr Principal Developer
M&E-Product Development Group
Autodesk, Inc.
www.autodesk.com http://www.autodesk.com/

This review is for code that parses files and is primarily used by introspective tools like llvm-objdump and the like. The code you are referring is substantially different, I think that discussion belongs in a different thread from this one.

[filcab]

Hi Benoit,

[belleyb]

Thanks for taking the time to answer. My comment was by no-mean implying that I was requesting any immediate code change. I completely agree, as I mentioned, in my original comment, that calling report_fatal_error() is infinitely better than crashing on segfault.

I was under the impression that the Object reader would be used when using/implementing an MCJIT object cache. Is that the case ?

Benoit

From: Filipe Cabecinhas <[e-mail]<mailto:[e-mail]>>
Date: jeudi 15 janvier 2015 14:17
To: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Cc: "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, Benoit Belley <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>, "[e-mail]<mailto:[e-mail]>" <[e-mail]<mailto:[e-mail]>>
Subject: Re: [PATCH] Report fatal errors instead of segfaulting/asserting on a few invalid accesses while reading MachO files.

Hi Benoit,

[• rafael]

Thanks again for fuzzing llvm!

lib/Object/MachOObjectFile.cpp

namespace {
   struct section_base {
     char sectname[16];
     char segname[16];
   };
 }
 
 template<typename T>
 static T getStruct(const MachOObjectFile *O, const char *P) {
+  // Don't read before the beginning or past the end of the file
+  if (P < O->getData().begin() || P + sizeof(T) > O->getData().end())
+    report_fatal_error("Malformed MachO file.");
+
   T Cmd;
   memcpy(&Cmd, P, sizeof(T));
   if (O->isLittleEndian() != sys::IsLittleEndianHost)
     MachO::swapStruct(Cmd);
   return Cmd;
 }
 
 static uint32_t
 getSegmentLoadCommandNumSections(const MachOObjectFile *O,
                                  const MachOObjectFile::LoadCommandInfo &L) {
   if (O->is64Bit()) {
+    const unsigned SectionSize = sizeof(MachO::section_64);
     MachO::segment_command_64 S = O->getSegment64LoadCommand(L);
+    if (S.nsects > std::numeric_limits<uint32_t>::max() / SectionSize ||
+        S.nsects * SectionSize > L.C.cmdsize - sizeof(S))
+      report_fatal_error(
+          "Number of sections too large for size of load command.");
     return S.nsects;
   }
+  const unsigned SectionSize = sizeof(MachO::section);
   MachO::segment_command S = O->getSegmentLoadCommand(L);
+  if (S.nsects > std::numeric_limits<uint32_t>::max() / SectionSize ||
+      S.nsects * SectionSize > L.C.cmdsize - sizeof(S))
+    report_fatal_error(
+        "Number of sections too large for size of load command.");
   return S.nsects;
 }
 
 static bool isPageZeroSegment(const MachOObjectFile *O,
                               const MachOObjectFile::LoadCommandInfo &L) {
   if (O->is64Bit()) {
     MachO::segment_command_64 S = O->getSegment64LoadCommand(L);
     return StringRef("__PAGEZERO").equals(S.segname);
for (unsigned I = 0; ; ++I) {
     } else if (Load.C.cmd == MachO::LC_UUID) {
       // Multiple UUID load commands
       if (UuidLoadCmd) {
         EC = object_error::parse_failed;
         return;
       }
       UuidLoadCmd = Load.Ptr;
     } else if (Load.C.cmd == SegmentLoadType) {
+      const unsigned SegmentLoadSize = this->is64Bit()
+                                           ? sizeof(MachO::segment_command_64)
+                                           : sizeof(MachO::segment_command);
+      if (Load.C.cmdsize < SegmentLoadSize)
+        report_fatal_error("Too small segment load command size.");
+
       uint32_t NumSections = getSegmentLoadCommandNumSections(this, Load);
       for (unsigned J = 0; J < NumSections; ++J) {
         const char *Sec = getSectionPtr(this, Load, J);
         Sections.push_back(Sec);
       }
       if (isPageZeroSegment(this, Load))
         HasPageZeroSegment = true;
     } else if (Load.C.cmd == MachO::LC_LOAD_DYLIB ||
void MachOObjectFile::moveSymbolNext(DataRefImpl &Symb) const {
   Symb.p += SymbolTableEntrySize;
 }
 
 std::error_code MachOObjectFile::getSymbolName(DataRefImpl Symb,
                                                StringRef &Res) const {
   StringRef StringTable = getStringTableData();
   MachO::nlist_base Entry = getSymbolTableEntryBase(this, Symb);
   const char *Start = &StringTable.data()[Entry.n_strx];
+  if (Start >= getData().end())
+    report_fatal_error("Symbol name entry points past end of file.");
   Res = StringRef(Start);
   return object_error::success;
 }
 
 unsigned MachOObjectFile::getSectionType(SectionRef Sec) const {
   DataRefImpl DRI = Sec.getRawDataRefImpl();
   uint32_t Flags = getSectionFlags(this, DRI);
   return Flags & MachO::SECTION_TYPE;

 }
 
 basic_symbol_iterator MachOObjectFile::getSymbolByIndex(unsigned Index) const {
   DataRefImpl DRI;
   if (!SymtabLoadCmd)
     return basic_symbol_iterator(SymbolRef(DRI, this));
 
   MachO::symtab_command Symtab = getSymtabLoadCommand();
-  assert(Index < Symtab.nsyms && "Requested symbol index is out of range.");
+  if (Index >= Symtab.nsyms)
+    report_fatal_error("Requested symbol index is out of range.");
   unsigned SymbolTableEntrySize =
     is64Bit() ? sizeof(MachO::nlist_64) : sizeof(MachO::nlist);
   DRI.p = reinterpret_cast<uintptr_t>(getPtr(this, Symtab.symoff));
   DRI.p += Index * SymbolTableEntrySize;
   return basic_symbol_iterator(SymbolRef(DRI, this));
 }
 
 section_iterator MachOObjectFile::section_begin() const {

 StringRef
 MachOObjectFile::getSectionFinalSegmentName(DataRefImpl Sec) const {
   ArrayRef<char> Raw = getSectionRawFinalSegmentName(Sec);
   return parseSegmentOrSectionName(Raw.data());
 }
 
 ArrayRef<char>
 MachOObjectFile::getSectionRawName(DataRefImpl Sec) const {
+  if (Sec.d.a >= Sections.size())
+    report_fatal_error("getSectionRawName: Invalid section index");
   const section_base *Base =
     reinterpret_cast<const section_base *>(Sections[Sec.d.a]);
   return makeArrayRef(Base->sectname);
 }
 
 ArrayRef<char>
 MachOObjectFile::getSectionRawFinalSegmentName(DataRefImpl Sec) const {
+  if (Sec.d.a >= Sections.size())
+    report_fatal_error("getSectionRawFinalSegmentName: Invalid section index");
   const section_base *Base =
     reinterpret_cast<const section_base *>(Sections[Sec.d.a]);
   return makeArrayRef(Base->segname);
 }
 
 bool
 MachOObjectFile::isRelocationScattered(const MachO::any_relocation_info &RE)
   const {

 MachOObjectFile::LoadCommandInfo
 MachOObjectFile::getFirstLoadCommandInfo() const {
   MachOObjectFile::LoadCommandInfo Load;
 
   unsigned HeaderSize = is64Bit() ? sizeof(MachO::mach_header_64) :
                                     sizeof(MachO::mach_header);
   Load.Ptr = getPtr(this, HeaderSize);
   Load.C = getStruct<MachO::load_command>(this, Load.Ptr);
+  if (Load.C.cmdsize < 8)
+    report_fatal_error("Load command with size < 8 bytes.");
   return Load;
 }
 
 MachOObjectFile::LoadCommandInfo
 MachOObjectFile::getNextLoadCommandInfo(const LoadCommandInfo &L) const {
   MachOObjectFile::LoadCommandInfo Next;
   Next.Ptr = L.Ptr + L.C.cmdsize;
   Next.C = getStruct<MachO::load_command>(this, Next.Ptr);
+  if (Next.C.cmdsize < 8)
+    report_fatal_error("Load command with size < 8 bytes.");
   return Next;
 }
 
 MachO::section MachOObjectFile::getSection(DataRefImpl DRI) const {
+  // TODO: What if Sections.size() == 0?
+  if (DRI.d.a >= Sections.size())
+    report_fatal_error("getSection: Invalid section index.");
   return getStruct<MachO::section>(this, Sections[DRI.d.a]);
 }
 
 MachO::section_64 MachOObjectFile::getSection64(DataRefImpl DRI) const {
+  // TODO: What if Sections.size() == 0?
+  if (DRI.d.a >= Sections.size())
+    report_fatal_error("getSection64: Invalid section index.");
   return getStruct<MachO::section_64>(this, Sections[DRI.d.a]);
 }
 
 MachO::section MachOObjectFile::getSection(const LoadCommandInfo &L,
                                            unsigned Index) const {
   const char *Sec = getSectionPtr(this, L, Index);
   return getStruct<MachO::section>(this, Sec);
 }

test/Object/macho-invalid.test

+// No crash, might not be totally invalid
+RUN: llvm-objdump -private-headers %p/Inputs/macho-invalid-zero-ncmds
+
+RUN: not llvm-objdump -private-headers %p/Inputs/macho-invalid-too-small-load-command 2>&1 \
+RUN:      | FileCheck -check-prefix SMALL-LOADC-SIZE %s
+RUN: not llvm-objdump -private-headers %p/Inputs/macho64-invalid-too-small-load-command 2>&1 \
+RUN:      | FileCheck -check-prefix SMALL-LOADC-SIZE %s
+
+RUN: not llvm-objdump -private-headers %p/Inputs/macho-invalid-no-size-for-sections 2>&1 \
+RUN:      | FileCheck -check-prefix TOO-MANY-SECTS %s
+RUN: not llvm-objdump -private-headers %p/Inputs/macho64-invalid-no-size-for-sections 2>&1 \
+RUN:      | FileCheck -check-prefix TOO-MANY-SECTS %s
+
+RUN: not llvm-objdump -t %p/Inputs/macho-invalid-bad-symbol-index 2>&1 \
+RUN:      | FileCheck -check-prefix BAD-SYMBOL %s
+
+RUN: not llvm-objdump -t %p/Inputs/macho-invalid-symbol-name-past-eof 2>&1 \
+RUN:      | FileCheck -check-prefix NAME-PAST-EOF %s
+
+RUN: not llvm-objdump -t %p/Inputs/macho-invalid-section-index 2>&1 \
+RUN:      | FileCheck -check-prefix INVALID-SEGMENT-SECTION-IDX %s
+
+
+SMALL-LOADC-SIZE: Too small segment load command size
+TOO-MANY-SECTS: Number of sections too large for size of load command
+BAD-SYMBOL: Requested symbol index is out of range
+NAME-PAST-EOF: Symbol name entry points past end of file
+
+INVALID-SEGMENT-SECTION-IDX: Invalid section index

test/Object/objdump-macho-quirks.test

-RUN: llvm-objdump -private-headers %p/Inputs/macho-zero-ncmds \
-RUN:         | FileCheck %s -check-prefix A
-
-// Check that we don't get an infinite loop if ncmds = 0
-A: file format Mach-O 64-bit unknown
-A: Mach header
-A:      magic cputype cpusubtype  caps    filetype ncmds sizeofcmds      flags
-A: MH_MAGIC_64  0x00      OBJECT     0          0 0x00000000
-