[llvm-readobj] - Do not crash when an invalid .eh_frame_hdr is dumped using --unwind.

[grimar]

Summary

When the p_offset/p_filesz of the PT_GNU_EH_FRAME is invalid
(e.g larger than the file size) then llvm-readobj might crash.

This patch fixes the issue. I've introduced ELFFile<ELFT>::getSegmentContent
method, which is very similar to ELFFile<ELFT>::getSectionContentsAsArray one.

[grimar]

llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h

113

Note: we might want to change this and all other reportError calls in the PrinterContext<ELFT> to report warnings.

[jhenderson]

llvm/include/llvm/Object/ELF.h

72

Twine instead of std::to_string?

73

Delete "the".

llvm/test/tools/llvm-readobj/ELF/unwind.test

287

"so large a value that it overflows" is slightly more grammatically correct, I think (English is weird - most native speakers don't even have a good grasp of all the rules, but this feels right!).

I wonder if we should have a 64-bit version of this same case, since it has a different upper bound?

llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h

113

Sounds reasonable to me, but probably a separate change.

[grimar]

• Addressed review comments.

llvm/test/tools/llvm-readobj/ELF/unwind.test

287

I wonder if we should have a 64-bit version of this same case, since it has a different upper bound?

Yes, probably.

It seems was not possible (or too complicated, I was unable to) to produce the right values for the program header with the previous approach (because of overflows happening), so I've also changed how tests are implemented. Now I set needed size/offset values directly for the PT_GNU_EH_FRAME. It is the most straightforward way to set them and I it also looks much cleaner than before I think. Not sure why I did not do it in this way from the begining.

I've also added a test (Case D) for "p_memsz does not match p_filesz for GNU_EH_FRAME" error (seems we do not have it anywhere, but it is related). I'd improve this message in a follow-up though to report actual values.

llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h

113

Yes. That is what I meant, it is in my TODO list.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/unwind.test

305

so large -> is so large

327

Perhaps you should just keep Case D in a separate patch, and improve the error text at the same time. It's not really related to the rest of the changes you are making, I think. Anyway, why is this an error at all? I'm not convinced it's really anything to do with dumping. It might be some sort of validation error if we had something equivalent to llvm-dwarfdump --verify, but I'm not convinced it belongs in llvm-readelf as things stand.

[grimar]

llvm/test/tools/llvm-readobj/ELF/unwind.test

327

Anyway, why is this an error at all?

It was introduced in D43313.
I guess it was just a some kind of sanity check. Having Phdr.p_memsz != Phdr.p_filesz probably means something is wrong with the header (e.g a bss section was included?). Perhaps we should just convert it to warning and use p_filesz when trying to printEHFrameHdr().

I'll remove "case D" and investigate it separatelly.

[grimar]

• Removed "Case D".
• Fixed comment.

[jhenderson]

LGTM.

llvm/include/llvm/Object/ELF.h

std::string getSecIndexForError(const ELFFile<ELFT> *Obj,
   // To make this helper be more convenient for error reporting purposes we
   // drop the error. But really it should never be triggered. Before this point,
   // our code should have called 'sections()' and reported a proper error on
   // failure.
   llvm::consumeError(TableOrErr.takeError());
   return "[unknown index]";
 }
 
+template <class ELFT>
+std::string getPhdrIndexForError(const ELFFile<ELFT> *Obj,
+                                 const typename ELFT::Phdr *Phdr) {
+  auto Headers = Obj->program_headers();
+  if (Headers)
+    return ("[index " + Twine(Phdr - &Headers->front()) + "]").str();
+  // See comment in the getSecIndexForError() above.
+  llvm::consumeError(Headers.takeError());
+  return "[unknown index]";
+}
+
 static inline Error defaultWarningHandler(const Twine &Msg) {
   return createError(Msg);
 }
 
 template <class ELFT>
 class ELFFile {
 public:
   LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
public:
   Expected<StringRef>
   getSectionName(const Elf_Shdr *Section,
                  WarningHandler WarnHandler = &defaultWarningHandler) const;
   Expected<StringRef> getSectionName(const Elf_Shdr *Section,
                                      StringRef DotShstrtab) const;
   template <typename T>
   Expected<ArrayRef<T>> getSectionContentsAsArray(const Elf_Shdr *Sec) const;
   Expected<ArrayRef<uint8_t>> getSectionContents(const Elf_Shdr *Sec) const;
+  Expected<ArrayRef<uint8_t>> getSegmentContents(const Elf_Phdr *Phdr) const;
 };
 
 using ELF32LEFile = ELFFile<ELF32LE>;
 using ELF64LEFile = ELFFile<ELF64LE>;
 using ELF32BEFile = ELFFile<ELF32BE>;
 using ELF64BEFile = ELFFile<ELF64BE>;
 
 template <class ELFT>
if (Offset % alignof(T))
     return createError("unaligned data");
 
   const T *Start = reinterpret_cast<const T *>(base() + Offset);
   return makeArrayRef(Start, Size / sizeof(T));
 }
 
 template <class ELFT>
 Expected<ArrayRef<uint8_t>>
+ELFFile<ELFT>::getSegmentContents(const Elf_Phdr *Phdr) const {
+  uintX_t Offset = Phdr->p_offset;
+  uintX_t Size = Phdr->p_filesz;
+
+  if (std::numeric_limits<uintX_t>::max() - Offset < Size)
+    return createError("program header " + getPhdrIndexForError(this, Phdr) +
+                       " has a p_offset (0x" + Twine::utohexstr(Offset) +
+                       ") + p_filesz (0x" + Twine::utohexstr(Size) +
+                       ") that cannot be represented");
+  if (Offset + Size > Buf.size())
+    return createError("program header  " + getPhdrIndexForError(this, Phdr) +
+                       " has a p_offset (0x" + Twine::utohexstr(Offset) +
+                       ") + p_filesz (0x" + Twine::utohexstr(Size) +
+                       ") that is greater than the file size (0x" +
+                       Twine::utohexstr(Buf.size()) + ")");
+  return makeArrayRef(base() + Offset, Size);
+}
+
+template <class ELFT>
+Expected<ArrayRef<uint8_t>>
 ELFFile<ELFT>::getSectionContents(const Elf_Shdr *Sec) const {
   return getSectionContentsAsArray<uint8_t>(Sec);
 }
 
 template <class ELFT>
 StringRef ELFFile<ELFT>::getRelocationTypeName(uint32_t Type) const {
   return getELFRelocationTypeName(getHeader()->e_machine, Type);
 }

llvm/test/tools/llvm-readobj/ELF/unwind.test

FileHeader:
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_386
 Sections:
   - Name:    .eh_frame
     Type:    SHT_PROGBITS
 ## Length is set to 0xFF, though the actual section length is 4.
     Content: "FF000000"
+
+## Check we report an error when we can't read the content of the .eh_frame section.
+
+## Case A: test we report an error when the p_offset of the PT_GNU_EH_FRAME
+##         is invalid (goes past the end of the file).
+
+# RUN: yaml2obj --docnum=4 %s -o %t4 -DOFFSET=0xffff0000 -DSIZE=0x1 -DBITS=32
+# RUN: not llvm-readobj --unwind %t4 2>&1 \
+# RUN:   | FileCheck %s -DFILE=%t4 --check-prefix=BROKEN-CONTENT -DOFFSET=0xffff0000 -DSIZE=0x1
+
+# RUN: yaml2obj --docnum=4 %s -o %t5 -DOFFSET=0x1 -DSIZE=0xffff0000 -DBITS=32
+# RUN: not llvm-readobj --unwind %t5 2>&1 \
+# RUN:   | FileCheck %s -DFILE=%t5 --check-prefix=BROKEN-CONTENT -DOFFSET=0x1 -DSIZE=0xffff0000
+
+# BROKEN-CONTENT:      EHFrameHeader {
+# BROKEN-CONTENT-NEXT:   Address: 0x0
+# BROKEN-CONTENT-NEXT:   Offset:  [[OFFSET]]
+# BROKEN-CONTENT-NEXT:   Size:    [[SIZE]]
+# BROKEN-CONTENT-NEXT:   Corresponding Section:
+# BROKEN-CONTENT-NEXT: error: '[[FILE]]': program header  [index 0] has a p_offset ([[OFFSET]]) + p_filesz ([[SIZE]]) that is greater than the file size (0xe0)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS[[BITS]]
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_NONE
+ProgramHeaders:
+  - Type: PT_GNU_EH_FRAME
+    MemSize:  [[SIZE]]
+    FileSize: [[SIZE]]
+    Offset:   [[OFFSET]]
+    Sections: []
+
+## Case B: test we report an error when the file size of the PT_GNU_EH_FRAME
+##         is invalid (goes past the end of the file).
+# RUN: yaml2obj --docnum=4 %s -o %t6 -DOFFSET=0x100 -DSIZE=0xffff0000 -DBITS=32
+# RUN: not llvm-readobj --unwind %t6 2>&1 \
+# RUN:   | FileCheck %s -DFILE=%t6 --check-prefix=BROKEN-CONTENT -DOFFSET=0x100 -DSIZE=0xffff0000
+
+## Case C: test we report an error when the offset + the file size of the PT_GNU_EH_FRAME is so large a
+##         value that it overflows the platform address size type.
+
+# RUN: yaml2obj --docnum=4 %s -o %t7 -DOFFSET=0x1 -DSIZE=0xffffffff -DBITS=32
+# RUN: not llvm-readobj --unwind %t7 2>&1 | FileCheck %s -DFILE=%t7 --check-prefix=BROKEN-CONTENT2 -DOFFSET=0x1 -DSIZE=0xffffffff
+
+# RUN: yaml2obj --docnum=4 %s -o %t8 -DOFFSET=0xffffffff -DSIZE=0x1 -DBITS=32
+# RUN: not llvm-readobj --unwind %t8 2>&1 | FileCheck %s -DFILE=%t8 --check-prefix=BROKEN-CONTENT2 -DOFFSET=0xffffffff -DSIZE=0x1
+
+# RUN: yaml2obj --docnum=4 %s -o %t9 -DOFFSET=0x1 -DSIZE=0xffffffffffffffff -DBITS=64
+# RUN: not llvm-readelf --unwind %t9 2>&1 | FileCheck %s -DFILE=%t9 --check-prefix=BROKEN-CONTENT2 -DOFFSET=0x1 -DSIZE=0xffffffffffffffff
+
+# RUN: yaml2obj --docnum=4 %s -o %t10 -DOFFSET=0xffffffffffffffff -DSIZE=0x1 -DBITS=64
+# RUN: not llvm-readelf --unwind %t10 2>&1 | FileCheck %s -DFILE=%t10 --check-prefix=BROKEN-CONTENT2 -DOFFSET=0xffffffffffffffff -DSIZE=0x1
+
+# BROKEN-CONTENT2:      EHFrameHeader {
+# BROKEN-CONTENT2-NEXT:   Address: 0x0
+# BROKEN-CONTENT2-NEXT:   Offset: [[OFFSET]]
+# BROKEN-CONTENT2-NEXT:   Size:   [[SIZE]]
+# BROKEN-CONTENT2-NEXT:   Corresponding Section:
+# BROKEN-CONTENT2-NEXT: error: '[[FILE]]': program header [index 0] has a p_offset ([[OFFSET]]) + p_filesz ([[SIZE]]) that cannot be represented

llvm/tools/llvm-readobj/DwarfCFIEHPrinter.h

 namespace llvm {
 namespace DwarfCFIEH {
 
 template <typename ELFT>
 class PrinterContext {
   ScopedPrinter &W;
   const object::ELFObjectFile<ELFT> *ObjF;
 
-  void printEHFrameHdr(uint64_t Offset, uint64_t Address, uint64_t Size) const;
-
+  void printEHFrameHdr(const typename ELFT::Phdr *EHFramePHdr) const;
   void printEHFrame(const typename ELFT::Shdr *EHFrameShdr) const;
 
 public:
   PrinterContext(ScopedPrinter &W, const object::ELFObjectFile<ELFT> *ObjF)
       : W(W), ObjF(ObjF) {}
 
   void printUnwindInformation() const;
 };
for (const auto &Shdr : *Sections)
     if (Shdr.sh_addr == Addr)
       return &Shdr;
   return nullptr;
 }
 
 template <typename ELFT>
 void PrinterContext<ELFT>::printUnwindInformation() const {
   const object::ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  const typename ELFT::Phdr *EHFramePhdr = nullptr;
 
   auto PHs = Obj->program_headers();
   if (Error E = PHs.takeError())
     reportError(std::move(E), ObjF->getFileName());
 
   for (const auto &Phdr : *PHs) {
     if (Phdr.p_type == ELF::PT_GNU_EH_FRAME) {
-      EHFramePhdr = &Phdr;
       if (Phdr.p_memsz != Phdr.p_filesz)
         reportError(object::createError(
                         "p_memsz does not match p_filesz for GNU_EH_FRAME"),
                     ObjF->getFileName());
+      printEHFrameHdr(&Phdr);
       break;
     }
   }
 
-  if (EHFramePhdr)
-    printEHFrameHdr(EHFramePhdr->p_offset, EHFramePhdr->p_vaddr,
-                    EHFramePhdr->p_memsz);
-
   auto Sections = Obj->sections();
   if (Error E = Sections.takeError())
     reportError(std::move(E), ObjF->getFileName());
 
   for (const auto &Shdr : *Sections) {
     auto SectionName = Obj->getSectionName(&Shdr);
     if (Error E = SectionName.takeError())
       reportError(std::move(E), ObjF->getFileName());
 
     if (*SectionName == ".eh_frame")
       printEHFrame(&Shdr);
   }
 }
 
 template <typename ELFT>
-void PrinterContext<ELFT>::printEHFrameHdr(uint64_t EHFrameHdrOffset,
-                                           uint64_t EHFrameHdrAddress,
-                                           uint64_t EHFrameHdrSize) const {
+void PrinterContext<ELFT>::printEHFrameHdr(const typename ELFT::Phdr *EHFramePHdr) const {
   DictScope L(W, "EHFrameHeader");
+  uint64_t EHFrameHdrAddress = EHFramePHdr->p_vaddr;
   W.startLine() << format("Address: 0x%" PRIx64 "\n", EHFrameHdrAddress);
-  W.startLine() << format("Offset: 0x%" PRIx64 "\n", EHFrameHdrOffset);
-  W.startLine() << format("Size: 0x%" PRIx64 "\n", EHFrameHdrSize);
+  W.startLine() << format("Offset: 0x%" PRIx64 "\n", (uint64_t)EHFramePHdr->p_offset);
+  W.startLine() << format("Size: 0x%" PRIx64 "\n", (uint64_t)EHFramePHdr->p_memsz);
 
   const object::ELFFile<ELFT> *Obj = ObjF->getELFFile();
-  const auto *EHFrameHdrShdr = findSectionByAddress(ObjF, EHFrameHdrAddress);
+  const typename ELFT::Shdr *EHFrameHdrShdr =
+      findSectionByAddress(ObjF, EHFramePHdr->p_vaddr);
   if (EHFrameHdrShdr) {
     auto SectionName = Obj->getSectionName(EHFrameHdrShdr);
     if (Error E = SectionName.takeError())
       reportError(std::move(E), ObjF->getFileName());
 
     W.printString("Corresponding Section", *SectionName);
   }
 
-  DataExtractor DE(makeArrayRef(Obj->base() + EHFrameHdrOffset, EHFrameHdrSize),
+  Expected<ArrayRef<uint8_t>> Content = Obj->getSegmentContents(EHFramePHdr);
+  if (!Content)
+    reportError(Content.takeError(), ObjF->getFileName());
+
+  DataExtractor DE(*Content,
                    ELFT::TargetEndianness == support::endianness::little,
                    ELFT::Is64Bits ? 8 : 4);
 
   DictScope D(W, "Header");
   uint64_t Offset = 0;
 
   auto Version = DE.getU8(&Offset);
   W.printNumber("version", Version);
void PrinterContext<ELFT>::printEHFrameHdr(const typename ELFT::Phdr *EHFramePHdr) const {
   auto EHFramePtr = DE.getSigned(&Offset, 4) + EHFrameHdrAddress + 4;
   W.startLine() << format("eh_frame_ptr: 0x%" PRIx64 "\n", EHFramePtr);
 
   auto FDECount = DE.getUnsigned(&Offset, 4);
   W.printNumber("fde_count", FDECount);
 
   unsigned NumEntries = 0;
   uint64_t PrevPC = 0;
-  while (Offset + 8 <= EHFrameHdrSize && NumEntries < FDECount) {
+  while (Offset + 8 <= EHFramePHdr->p_memsz && NumEntries < FDECount) {
     DictScope D(W, std::string("entry ")  + std::to_string(NumEntries));
 
     auto InitialPC = DE.getSigned(&Offset, 4) + EHFrameHdrAddress;
     W.startLine() << format("initial_location: 0x%" PRIx64 "\n", InitialPC);
     auto Address = DE.getSigned(&Offset, 4) + EHFrameHdrAddress;
     W.startLine() << format("address: 0x%" PRIx64 "\n", Address);
 
     if (InitialPC < PrevPC)