[llvm-objcopy] --only-keep-debug: place zero-size segment according to its parent segment

[MaskRay]

Summary

Alternative to D74755. sectionWithinSegment() treats an empty section as having
a size of 1. Due to the rule, an empty .tdata will not be attributed to an
empty PT_TLS. (The empty p_align=64 PT_TLS is for Android Bionic's TCB
compatibility (ELF-TLS). See https://reviews.llvm.org/D62055#1507426)

Currently --only-keep-debug will not layout a segment with no section
(layoutSegmentsForOnlyKeepDebug()), thus p_offset of PT_TLS can go past the end
of the file. The strange p_offset can trigger validation errors for subsequent
tools, e.g. llvm-objcopy errors when reading back the separate debug file
(readProgramHeaders()).

This patch places such an empty segment according to its parent segment. This
special cases works for the empty PT_TLS used in Android. For a non-empty
segment, it should have at least one non-empty section and will be handled by
the normal code.

[jhenderson]

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too? In such a case, the PT_TLS segment won't be nested inside anything (see segmentOverlapsSegment).

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

232

246

It's not clear from the test comments why we need the .got, .note and .text sections in this output. Either remove them or add comments explaining their purpose.

271

Note: @grimar is working on D90458 which changes how sections within segments are represented.

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

2314

Do you mean offset here?

[MaskRay]

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

In such a case, the PT_TLS segment won't be nested inside anything (see segmentOverlapsSegment).

Where PT_TLS is nested is an insignificant detail. For --only-keep-debug, the program headers are retained just so that address mapping can work without access to the original program headers. For other program headers, it is rather unimportant whether they are mapped.

[MaskRay]

Comments

[MaskRay]

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

246

Added a comment.

271

I can rebase.

[jhenderson]

In D90897#2379198, @MaskRay wrote:

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.

In such a case, the PT_TLS segment won't be nested inside anything (see segmentOverlapsSegment).

Where PT_TLS is nested is an insignificant detail. For --only-keep-debug, the program headers are retained just so that address mapping can work without access to the original program headers. For other program headers, it is rather unimportant whether they are mapped.

I only used PT_TLS here as an example. The same would apply for any zero-sized segment unless it appeared in the middle or at the start of a non-empty segment. The point being that in such a case, it will contain no sections nor will it be nested inside a segment, and so the offset will not get changed by my understanding. This in turn could cause the issue this patch is trying to address.

One thought: how about an empty segment without a parent segment have its offset set to either 0 or MaxOffset? That would mean the segment offset isn't outside the file, which would avoid the issue. If the offset of segments in an --only-keep-debug output is unimportant, that would work, I think? Might need some more thought to figure out if there were any other problems there.

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

2306–2307

This comment needs tweaking slightly, since the offset of empty segments is now potentially changing.

[MaskRay]

In D90897#2385176, @jhenderson wrote:

In D90897#2379198, @MaskRay wrote:

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

It is both Linux and FreeBSD (according to rL288808). LLD has removeEmptyPTLoad.

This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.

In such a case, the PT_TLS segment won't be nested inside anything (see segmentOverlapsSegment).

Where PT_TLS is nested is an insignificant detail. For --only-keep-debug, the program headers are retained just so that address mapping can work without access to the original program headers. For other program headers, it is rather unimportant whether they are mapped.

I only used PT_TLS here as an example. The same would apply for any zero-sized segment unless it appeared in the middle or at the start of a non-empty segment. The point being that in such a case, it will contain no sections nor will it be nested inside a segment, and so the offset will not get changed by my understanding. This in turn could cause the issue this patch is trying to address.

One thought: how about an empty segment without a parent segment have its offset set to either 0 or MaxOffset? That would mean the segment offset isn't outside the file, which would avoid the issue. If the offset of segments in an --only-keep-debug output is unimportant, that would work, I think? Might need some more thought to figure out if there were any other problems there.

Maybe. However, this does not practically exist so I don't think it is important to handle it.
(An alternative is to change the program header type to PT_NULL and don't error in ELFBuilder<ELFT>::readProgramHeaders, but we may have to fix other binary manipulation tools if pity)

--only-keep-debug is a very specific practical feature with very strong needs. I write the Seg->MemSize == 0 condition because in D74755 we have concluded that it is unclear how to handle such a segment. p_memsz!=0 does not need the rule. I am not sure why we want to emphasize p_memsz=0 PT_LOAD which does not practically exist. For an invalid (or at least, non-real) binary, why do we care so much about theoretical beauty for such a practical feature? (PT_LOAD is useful for mapping addresses. Other PT_LOAD are mostly useless. My first attempt adding --only-keep-debug did not even try adding program headers at all until many folks expressed favor for program headers)

I try to make the p_memsz=0 code simple before we have a good theory supporting empty segments. Why do we want to add more code for non-practical cases?

[MaskRay]

Adjust comments

[jhenderson]

LGTM - I realised last night whilst thinking about this that we can always enhance this further if the need arises - this change doesn't degrade the experience for any user, whilst improving it for the majority of use-cases.

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

2313

"no containing section" implies to me that there is no section that is a parent of the segment.

[MaskRay]

Thanks!

[MaskRay]

Fix a comment

[enh]

In D90897#2385929, @MaskRay wrote:

In D90897#2385176, @jhenderson wrote:

In D90897#2379198, @MaskRay wrote:

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

It is both Linux and FreeBSD (according to rL288808). LLD has removeEmptyPTLoad.

This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.

that might have been sensible ... but POSIX actually _requires_ that an mmap() of length 0 fails with EINVAL (it's a "shall fail", not just a "may fail": https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html).

[jhenderson]

In D90897#2392316, @enh wrote:

In D90897#2385929, @MaskRay wrote:

In D90897#2385176, @jhenderson wrote:

In D90897#2379198, @MaskRay wrote:

In D90897#2378385, @jhenderson wrote:

Won't this fail if the PT_LOAD the PT_TLS should be in has a zero size too?

PT_LOAD with p_memsz is invalid on Linux. Attempting to mmap a zero-sized segment will fail.

It is both Linux and FreeBSD (according to rL288808). LLD has removeEmptyPTLoad.

This is an implementation detail that won't necessarily be true for all systems. Kernels could also quite happily ignore zero-sized segments with a trivial piece of code to check the size before calling mmap.

that might have been sensible ... but POSIX actually _requires_ that an mmap() of length 0 fails with EINVAL (it's a "shall fail", not just a "may fail": https://pubs.opengroup.org/onlinepubs/9699919799/functions/mmap.html).

Right. What I was referring to was that a kernel which uses mmap to map PT_LOAD segments into memory could have a simple if(p_memsz != 0) clause to skip the call of mmap in the first place. So this wasn't a reference to what mmap may or may not do, it was a reference to what the kernel might do when faced with a PT_LOAD of size 0.

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

- Name:  .symtab
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
     Link:  .strtab
   - Name:  .strtab
     Type:  SHT_STRTAB
     Flags: [ SHF_ALLOC ]
 DynamicSymbols: []
 Symbols: []
+
+## PT_TLS and .tdata are empty. Test that we set its p_offset to the parent
+## segment's p_offset. If we don't rewrite the p_offset of PT_TLS and the deleted
+## bytes are large, p_offset can be larger than the file size, and trigger
+## validation errors with subsequent tools.
+# RUN: yaml2obj --docnum=4 %s -o %t4
+# RUN: llvm-objcopy --only-keep-debug %t4 %t4.dbg
+# RUN: llvm-readelf -S -l %t4.dbg | FileCheck --check-prefix=CHECK4 %s
+
+# CHECK4:      [Nr] Name        Type     Address          Off    Size   ES Flg Lk Inf Al
+# CHECK4:      [ 1] .text       NOBITS   0000000000000200 000200 000001 00  AX  0   0  0
+# CHECK4-NEXT: [ 2] .tdata      NOBITS   0000000000001240 000240 000000 00 WAT  0   0 64
+# CHECK4-NEXT: [ 3] .got        NOBITS   0000000000001240 000240 000008 00  WA  0   0  0
+
+# CHECK4:      Type Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
+# CHECK4-NEXT: LOAD 0x000200 0x0000000000000200 0x0000000000000200 0x000000 0x000001 R E 0x1000
+# CHECK4-NEXT: LOAD 0x000240 0x0000000000001240 0x0000000000001240 0x000000 0x000008 RW  0x1000
+# CHECK4-NEXT: TLS  0x000240 0x0000000000001240 0x0000000000001240 0x000000 0x000000 R   0x40
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:         .text
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:      0x200
+    Size:         1
+  - Name:         .tdata
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_ALLOC, SHF_WRITE, SHF_TLS ]
+    Address:      0x1240   # Ensure Address=0x1000+Offset
+    AddressAlign: 0x40
+  - Name:         .got
+    Type:         SHT_PROGBITS
+    Flags:        [ SHF_ALLOC, SHF_WRITE ]
+    Size:         8
+ProgramHeaders:
+  - Type:     PT_LOAD
+    Flags:    [ PF_R, PF_X ]
+    VAddr:    0x200
+    Align:    0x1000
+    FirstSec: .text
+    LastSec:  .text
+  ## Add .got so that the PT_LOAD does not have zero p_memsz. We don't add
+  ## sections to zero-sized segments so zero-sized segments may have strange
+  ## offsets. In practice, the Linux kernel errors when mmapping a p_memsz
+  ## PT_LOAD,so for practical so this assumption can generally be made.
+  - Type:     PT_LOAD
+    Flags:    [ PF_R, PF_W ]
+    VAddr:    0x1240
+    Align:    0x1000
+    FirstSec: .tdata
+    LastSec:  .got
+  - Type:     PT_TLS
+    Flags:    [ PF_R ]
+    VAddr:    0x1240
+    FirstSec: .tdata
+    LastSec:  .tdata

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

if (!FirstSec) {
       // sh_offset for non-SHF_ALLOC sections.
       Off = Sec.OriginalOffset - FirstSec->OriginalOffset + FirstSec->Offset;
     }
     Sec.Offset = Off;
     Off += Sec.Size;
   }
   return Off;
 }
 
-// Rewrite p_offset and p_filesz of non-empty non-PT_PHDR segments after
-// sh_offset values have been updated.
+// 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)
       continue;
 
     uint64_t Offset = FirstSec->Offset;
     uint64_t FileSize = 0;
     for (const SectionBase *Sec : Seg->Sections) {
       uint64_t Size = Sec->Type == SHT_NOBITS ? 0 : Sec->Size;