[llvm-objcopy] Fix unaligned p_offset after copy.
#79889
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Dmitriy Chestnykh (chestnykh) Changes
Full diff: https://github.com/llvm/llvm-project/pull/79889.diff 2 Files Affected:
diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.cpp b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
index c8b66d6fcb5ebfa..a2fb5f321af207a 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
@@ -2280,6 +2280,7 @@ static uint64_t layoutSegments(std::vector<Segment *> &Segments,
// segments. So we can simply layout segments one after the other accounting
// for alignment.
for (Segment *Seg : Segments) {
+ uint64_t SegAlign = std::max<uint64_t>(Seg->Align, 1);
// We assume that segments have been ordered by OriginalOffset and Index
// such that a parent segment will always come before a child segment in
// OrderedSegments. This means that the Offset of the ParentSegment should
@@ -2287,10 +2288,10 @@ static uint64_t layoutSegments(std::vector<Segment *> &Segments,
if (Seg->ParentSegment != nullptr) {
Segment *Parent = Seg->ParentSegment;
Seg->Offset =
- Parent->Offset + Seg->OriginalOffset - Parent->OriginalOffset;
+ alignTo(Parent->Offset, SegAlign) + Seg->OriginalOffset - Parent->OriginalOffset;
} else {
Seg->Offset =
- alignTo(Offset, std::max<uint64_t>(Seg->Align, 1), Seg->VAddr);
+ alignTo(Offset, SegAlign, Seg->VAddr);
}
Offset = std::max(Offset, Seg->Offset + Seg->FileSize);
}
diff --git a/llvm/test/tools/llvm-objcopy/ELF/aligned-offset.test b/llvm/test/tools/llvm-objcopy/ELF/aligned-offset.test
new file mode 100644
index 000000000000000..e9caa565dadc317
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/ELF/aligned-offset.test
@@ -0,0 +1,124 @@
+# This test tests that PT_LOAD segment has properly aligned p_offset
+# in the presented layout (The presense of PT_PHDR and PT_INTERP is important).
+
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj --program-headers %t2 | FileCheck %s
+
+!ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_DYN
+ Machine: EM_X86_64
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x1000
+ Size: 4096
+ - Name: .text2
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x1000
+ Size: 0x1938c9
+ - Name: .text3
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x1000
+ Size: 4096
+ - Name: .text4
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x1000
+ Size: 4096
+ - Name: .text5
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x1000
+ Size: 1658880
+ProgramHeaders:
+ - Type: PT_PHDR
+ Flags: [ PF_R ]
+ Offset: 0x40
+ VAddr: 0x40
+ PAddr: 0x40
+ FileSize: 0x268
+ MemSize: 0x268
+ Align: 0x8
+ - Type: PT_INTERP
+ Flags: [ PF_R ]
+ Offset: 0x195000
+ VAddr: 0x195000
+ PAddr: 0x195000
+ FileSize: 0xf
+ MemSize: 0xf
+ Align: 0x1
+ - Type: PT_LOAD
+ Flags: [ PF_X, PF_R ]
+ FirstSec: .text
+ LastSec: .text2
+ FileSize: 0x1948c9
+ MemSize: 0x1948c9
+ Offset: 0
+ - Type: PT_LOAD
+ Flags: [ PF_R ]
+ FirstSec: .text3
+ LastSec: .text4
+ FileSize: 0xdf960
+ MemSize: 0xdf960
+ Offset: 0x195000
+ VAddr: 0x195000
+ PAddr: 0x195000
+
+#CHECK: ProgramHeaders [
+#CHECK-NEXT: ProgramHeader {
+#CHECK-NEXT: Type: PT_PHDR (0x6)
+#CHECK-NEXT: Offset: 0x40
+#CHECK-NEXT: VirtualAddress: 0x40
+#CHECK-NEXT: PhysicalAddress: 0x40
+#CHECK-NEXT: FileSize: 616
+#CHECK-NEXT: MemSize: 616
+#CHECK-NEXT: Flags [ (0x4)
+#CHECK-NEXT: PF_R (0x4)
+#CHECK-NEXT: ]
+#CHECK-NEXT: Alignment: 8
+#CHECK-NEXT: }
+#CHECK-NEXT: ProgramHeader {
+#CHECK-NEXT: Type: PT_INTERP (0x3)
+#CHECK-NEXT: Offset: 0x1948C9
+#CHECK-NEXT: VirtualAddress: 0x195000
+#CHECK-NEXT: PhysicalAddress: 0x195000
+#CHECK-NEXT: FileSize: 15
+#CHECK-NEXT: MemSize: 15
+#CHECK-NEXT: Flags [ (0x4)
+#CHECK-NEXT: PF_R (0x4)
+#CHECK-NEXT: ]
+#CHECK-NEXT: Alignment: 1
+#CHECK-NEXT: }
+#CHECK-NEXT: ProgramHeader {
+#CHECK-NEXT: Type: PT_LOAD (0x1)
+#CHECK-NEXT: Offset: 0x0
+#CHECK-NEXT: VirtualAddress: 0x0
+#CHECK-NEXT: PhysicalAddress: 0x0
+#CHECK-NEXT: FileSize: 1657033
+#CHECK-NEXT: MemSize: 1657033
+#CHECK-NEXT: Flags [ (0x5)
+#CHECK-NEXT: PF_R (0x4)
+#CHECK-NEXT: PF_X (0x1)
+#CHECK-NEXT: ]
+#CHECK-NEXT: Alignment: 4096
+#CHECK-NEXT: }
+#CHECK-NEXT: ProgramHeader {
+#CHECK-NEXT: Type: PT_LOAD (0x1)
+#CHECK-NEXT: Offset: 0x195000
+#CHECK-NEXT: VirtualAddress: 0x195000
+#CHECK-NEXT: PhysicalAddress: 0x195000
+#CHECK-NEXT: FileSize: 915808
+#CHECK-NEXT: MemSize: 915808
+#CHECK-NEXT: Flags [ (0x4)
+#CHECK-NEXT: PF_R (0x4)
+#CHECK-NEXT: ]
+#CHECK-NEXT: Alignment: 4096
+#CHECK-NEXT: }
+#CHECK-NEXT: ]
|
chestnykh
force-pushed
the
fix-p_offset-align
branch
from
5e0e4a0 to
cbd6f81
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
- When copying the ELF whose layout was described in the test the llvm-objcopy breaks the last PT_LOAD segment alignment of p_offset by assignment the `p_filesz` value of prev segment to it. This patch fixes this case. llvm#79887
chestnykh
force-pushed
the
fix-p_offset-align
branch
from
cbd6f81 to
c95797b
Compare
There was a problem hiding this comment.
I probably need to give some more thought, but I'm fairly confident the proposed fix is incorrect, as it would result in a segment that has a parent (i.e. one where there is some overlap with another segment) moving independently of its parent. Note that according to the comment immediately above the main change in this PR that states that a parent's offset should have already been assigned (and therefore will be correctly aligned), before this point. My suspicion (but I haven't checked) is that the algorithm for ordering and determining parent/child relationship is incorrect and/or the method to determine the alignment to use is incorrect. In particular, I suspect that the PT_INTERP segment in your original case is being treated as the parent of the second PT_LOAD and therefore its p_align value is the one being used to align the PT_INTERP, which in turn means the second PT_LOAD is also moved with PT_INTERP.
I think the correct fix would be to use the highest alignment in a set of overlapping segments when aligning the first segment in that list, adjusting its position from the "aligned" position according to how far off from the aligned value it was before copying. Alternatively, we treat the parent segment as the highest-alignment section, and adjust the other segments accordingly with it. There are some intricacies involved in both cases that we need to be careful of though.
|
Okay, I'll try to make the correct fix |
p_fileszvalue of prev segment to it. This patch fixes this case. llvm-objcopy produces wrongp_offsetwhen PT_INTERP/PT_LOAD offsets are equal #79887