[llvm-objcopy][ELF] --only-keep-debug: set offset/size of segments with no sections to zero

[MaskRay]

Summary

PR50160: we currently ignore non-PT_PHDR segments with no sections, not
accounting for its p_offset and p_filesz: this can cause an out-of-bounds write
in writeSegmentData if the p_offset+p_filesz is larger than the total file
size.

This can be fixed by setting p_offset=p_filesz=0. The logic nicely unifies with
the logic added in D90897.

[jhenderson]

This change makes some sense to me in itself (IIRC GNU objcopy sets segment offsets to 0 for empty segments even without --only-keep-debug), but I don't think the test quite covers the same case as @rupprecht's bug report? In that report, the sections have been stripped. Maybe it doesn't matter, but maybe we need a separate test case for --strip-sections (or equivalent) followed separately by --only-keep-debug?

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

284

Any particular reason for the double blank line?

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

2323

[MaskRay]

Add a test

[jhenderson]

LGTM, but probably would be useful for someone else (especially @rupprecht) to take a quick look.

llvm/test/tools/llvm-objcopy/ELF/only-keep-debug.test

 ## it is followed by .note which is not SHT_NOBITS.
 # CHECK2:      [Nr] Name        Type     Address          Off    Size   ES Flg Lk Inf Al
 # CHECK2:      [ 1] .text       NOBITS   0000000000000200 000200 000001 00  AX  0   0 512
 # CHECK2-NEXT: [ 2] .note       NOTE     0000000000000201 000201 000001 00   A  0   0  0
 # CHECK2-NEXT: [ 3] .debug_info PROGBITS 0000000000000000 000220 000001 00      0   0 32
 # CHECK2-NEXT: [ 4] .strtab     STRTAB   0000000000000000 000221 000001 00      0   0  1
 # CHECK2-NEXT: [ 5] .shstrtab   STRTAB   0000000000000000 000222 00002b 00      0   0  1
 
-## Check that p_offset or p_filesz of empty segments or PT_PHDR are not modified.
+## Check that p_offset or p_filesz of PT_PHDR are not modified.
 # CHECK2:      Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
 # CHECK2-NEXT: PHDR 0x000040 0x0000000000000040 0x0000000000000040 0x0000a8 0x0000a8 R   0x8
 # CHECK2-NEXT: LOAD 0x000000 0x0000000000000000 0x0000000000000000 0x000202 0x000202 R E 0x1000
-# CHECK2-NEXT: LOAD 0x000202 0x0000000000000202 0x0000000000000202 0x00000e 0x00000e RW  0x1
+# CHECK2-NEXT: LOAD 0x000000 0x0000000000000202 0x0000000000000202 0x000000 0x00000e RW  0x1
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_DYN
   Machine: EM_X86_64
 Sections:
- Type:     PT_LOAD
     Align:    0x1000
     FirstSec: .tdata
     LastSec:  .got
   - Type:     PT_TLS
     Flags:    [ PF_R ]
     VAddr:    0x1240
     FirstSec: .tdata
     LastSec:  .tdata
+
+
+## The offset and size fields of segments which contain no section and have no
+## parent segment are set to zeros, so that we can decrease the file size. Such
+## segments are not useful for debugging.
+# RUN: yaml2obj --docnum=5 %s -o %t5
+# RUN: llvm-objcopy --only-keep-debug %t5 %t5.dbg
+# RUN: llvm-readelf -S -l %t5.dbg | FileCheck --check-prefix=CHECK5 %s
+
+# CHECK5:      [Nr] Name        Type     Address          Off    Size   ES Flg Lk Inf Al
+# CHECK5:      [ 1] .foo        NOBITS   0000000000000000 000078 001000 00   A  0   0  0
+# CHECK5-NEXT: [ 2] .strtab     STRTAB   0000000000000000 000078 000001 00      0   0  1
+# CHECK5-NEXT: [ 3] .shstrtab   STRTAB   0000000000000000 000079 000018 00      0   0  1
+
+# CHECK5:      Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK5-NEXT: NULL 0x000000 0x0000000000000000 0x0000000000000000 0x000078 0x000000     0x1
+# CHECK5-EMPTY:
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+Sections:
+  - Name:    .foo
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC ]
+    Size:    0x01000
+ProgramHeaders:
+  - Type:     PT_NULL
+    Flags:    []
+    FileSize: 0x01000

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

 }
 
 // Rewrite p_offset and p_filesz of non-PT_PHDR segments after sh_offset values
 // have been updated.
 static uint64_t layoutSegmentsForOnlyKeepDebug(std::vector<Segment *> &Segments,
                                                uint64_t HdrEnd) {
   uint64_t MaxOffset = 0;
   for (Segment *Seg : Segments) {
-    // An empty segment contains no section (see sectionWithinSegment). If it
-    // has a parent segment, copy the parent segment's offset field. This works
-    // for empty PT_TLS. We don't handle empty segments without a parent for
-    // now.
-    if (Seg->ParentSegment != nullptr && Seg->MemSize == 0)
-      Seg->Offset = Seg->ParentSegment->Offset;
-
-    const SectionBase *FirstSec = Seg->firstSection();
-    if (Seg->Type == PT_PHDR || !FirstSec)
+    if (Seg->Type == PT_PHDR)
       continue;
 
-    uint64_t Offset = FirstSec->Offset;
+    // The segment offset is generally the offset of the first section.
+    //
+    // For an empty segment contains no section (see sectionWithinSegment). If
+    // it has a parent segment, copy the parent segment's offset field. This
+    // works for empty PT_TLS. If no parent segment, use 0: the segment is
+    // not useful for debugging.
+    const SectionBase *FirstSec = Seg->firstSection();
+    uint64_t Offset =
+        FirstSec ? FirstSec->Offset
+                 : (Seg->ParentSegment ? Seg->ParentSegment->Offset : 0);
     uint64_t FileSize = 0;
     for (const SectionBase *Sec : Seg->Sections) {
       uint64_t Size = Sec->Type == SHT_NOBITS ? 0 : Sec->Size;
       if (Sec->Offset + Size > Offset)
         FileSize = std::max(FileSize, Sec->Offset + Size - Offset);
     }
 
     // If the segment includes EHDR and program headers, don't make it smaller