[PATCH] D42872: Fix handling of zero-size segments in llvm-objcopy

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added a comment.

In https://reviews.llvm.org/D42872#1001710, @jhenderson wrote:

> treat the ELF header and program header table as pseudo-segments for layout purposes, with an OriginalOffset of 0 and sizeof(ElfHeader) respectively. If they end up nested inside another segment, that's fine, as the segment by definition must be already in the right place. If they end up with segments nested inside them, that is also fine for the same reasoning. Zero-sized segments at offset 0 will still be placed at offset 0, as desired, but the ELF header segment will "follow" (at offset 0) immediately after it, since it will be the first non-empty segment in the ordered list (or will be nested inside another segment that is similarly at offset 0).
>
> How does this sound as an approach? It feels more robust and avoids any special casing, since everything should just naturally work with the existing layout scheme (the only thing we'd have to do is write these pseudo-segments differently).


Ah this is the right way to handle PT_PHDR! I love this solution. 1) The initial offset is always zero 2) create two new program headers (giving them proper ParentSegments) 3) add them to ordered segments 4) Use the information from those headers to set values in the ELF header. This also properlly handles another issue I hadn't even thought about. If a program expects the program headers to be loaded and the program headers don't immediately follow the ELF header the current

It will be a little while before I can get to this though. Should we patch this for now and add a TODO?

In https://reviews.llvm.org/D42872#1001677, @vit9696 wrote:

> I am personally not sure if it is the correct solution, because I do not understand why a segment with 0 filesize and a 0 offset should corrupt an ELF while copying regardless of its type.
>  I would personally prefer special-casing two segments to writing a check against both MemSize and FileSize, but it does not feel nice to me.
>  More importantly, if you have a PT_LOAD "marker" segment with all zero values:
>
> - will gnu binutils objcopy also corrupt such an ELF?
> - will a Linux/BSD-based system load such an ELF? If in both cases the answer is no, then we should probably just special-case indeed, but I still cannot say that silently overwriting the header is any good. Perhaps we should print an error and abort?


I think James' solution is the right way to handle this issue. As I stated I thought there were two issues before 1) I handle PT_PHDR incorrectly which causes this issue and 2) I assign invalid values to marker segments. James's solution resolves this issue 2) by avoiding special casing "marker" segments and solves issue 1) by having layout treat them the same way. So *really* the issue is that I didn't handle PT_PHDR segments the right way and this just happens to be how it blew up. Would you feel comfortable implementing James' solution? If not I'll eventually get around to it. Otherwise I'm happy temporarily using the "isMarker" special casing for PT_GNU_STACK and  PT_OPENBSD_WXNEEDED along with a "TODO" assigned to me to implement James's solution.


Repository:
  rL LLVM

https://reviews.llvm.org/D42872