[llvm-objcopy] - Emit error and don't crash if program header reaches past end of file.

[grimar]

Summary

This is https://bugs.llvm.org/show_bug.cgi?id=42122.

If an object file has a size less than program header's file [offset + size]
(i.e. if we have overflow), llvm-objcopy crashes instead of reporting a
error.

The patch fixes this issue.

[grimar]

• Removed excessive props from yaml.

[jhenderson]

test/tools/llvm-objcopy/ELF/invalid-p_filesz.test

7 ↗

(On Diff #203117)

Could the numbers be in hex? I feel like those are more natural.

Also, there are a couple of typos. 1) FIleCheck has a capital 'I' in it spuriously, and "mailformed" should be "malformed".

I'd probably change the message slightly to say "with offset 0x1234 and file size ..." (because the program header is not at an offset, it has an offset field, and there are two sizes in program headers).

20 ↗

(On Diff #203117)

I think we need another test case here, where the file offset is outside the file entirely.

tools/llvm-objcopy/ELF/Object.cpp

1109 ↗

(On Diff #203117)

mailformed -> malformed (as noted above).

[grimar]

• Addressed review comments.

test/tools/llvm-objcopy/ELF/invalid-p_filesz.test

7 ↗

(On Diff #203117)

Yeah.. I guess I wanted to start from "program segment at offset ..."

[jhenderson]

test/tools/llvm-objcopy/ELF/invalid-p_filesz-p_offset.test

24 ↗

(On Diff #203128)

now -> now the
of program -> of the program

tools/llvm-objcopy/ELF/Object.cpp

1110 ↗

(On Diff #203128)

Maybe rather than "is malformed" it might be better to say "goes past the end of the file" or something similar.

[jakehehrlich]

"goes past the end of the file" is the phrasing that I've seen GNU tools use and gets more to the point so I agree with James. Other than that this looks good to me!

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

llvm/trunk/test/tools/llvm-objcopy/ELF/invalid-p_filesz-p_offset.test

+## In this case, we have a program header with a file size that
+## overflows the binary size. Check llvm-objcopy doesn't crash
+## and report this error properly.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
+# RUN: not llvm-objcopy %t1.o 2>&1 | FileCheck %s --check-prefix=ERR1
+# ERR1: error: program header with offset 0x1b8 and file size 0x100000 goes past the end of the file
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+ProgramHeaders:
+  - Type:        PT_LOAD
+    FileSize:    0x100000
+    Sections:    
+      - Section: .foo
+
+## A similar case, but now the p_offset property of the program header is too large.
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: not llvm-objcopy %t2.o 2>&1 | FileCheck %s --check-prefix=ERR2
+# ERR2: error: program header with offset 0x100000 and file size 0x1 goes past the end of the file
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name: .foo
+    Type: SHT_PROGBITS
+    Size: 1
+ProgramHeaders:
+  - Type:        PT_LOAD
+    Offset:      0x100000
+    FileSize:    1
+    Sections:
+      - Section: .foo

llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp

if (&Child != &Parent && segmentOverlapsSegment(Child, Parent)) {
         }
     }
   }
 }
 
 template <class ELFT> void ELFBuilder<ELFT>::readProgramHeaders() {
   uint32_t Index = 0;
   for (const auto &Phdr : unwrapOrError(ElfFile.program_headers())) {
+    if (Phdr.p_offset + Phdr.p_filesz > ElfFile.getBufSize())
+      error("program header with offset 0x" + Twine::utohexstr(Phdr.p_offset) +
+            " and file size 0x" + Twine::utohexstr(Phdr.p_filesz) +
+            " goes past the end of the file");
+
     ArrayRef<uint8_t> Data{ElfFile.base() + Phdr.p_offset,
                            (size_t)Phdr.p_filesz};
     Segment &Seg = Obj.addSegment(Data);
     Seg.Type = Phdr.p_type;
     Seg.Flags = Phdr.p_flags;
     Seg.OriginalOffset = Phdr.p_offset;
     Seg.Offset = Phdr.p_offset;
     Seg.VAddr = Phdr.p_vaddr;