[llvm-objcopy] --gap-fill and 0-size sections

[quic-akaryaki]

In the change that added --gap-fill, the condition to choose the sections to write in BinaryWriter::write() did not exclude zero-size sections. However, zero-size sections did not have correct offsets assigned in BinaryWriter::finalize(). The result is either a failed assertion, or memory corruption due to writing to the buffer beyond its size.
To fix this, exclude zero-size sections and add a zero-size section to the test, which would trigger the bug.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Author: None (quic-akaryaki)

Changes

In the change that added --gap-fill, the condition to choose the sections to write in BinaryWriter::write() did not exclude zero-size sections. However, zero-size sections did not have correct offsets assigned in BinaryWriter::finalize(). The result is either a failed assertion, or memory corruption due to writing to the buffer beyond its size.
To fix this, exclude zero-size sections and add a zero-size section to the test, which would trigger the bug.

---

Full diff: https://github.com/llvm/llvm-project/pull/75837.diff

2 Files Affected:

• (modified) llvm/lib/ObjCopy/ELF/ELFObject.cpp (+1-1)
• (modified) llvm/test/tools/llvm-objcopy/ELF/gap-fill.test (+5)

diff --git a/llvm/lib/ObjCopy/ELF/ELFObject.cpp b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
index 5352736bdcb9b8..c8b66d6fcb5ebf 100644
--- a/llvm/lib/ObjCopy/ELF/ELFObject.cpp
+++ b/llvm/lib/ObjCopy/ELF/ELFObject.cpp
@@ -2638,7 +2638,7 @@ template <class ELFT> Error ELFWriter<ELFT>::finalize() {
 Error BinaryWriter::write() {
   SmallVector<const SectionBase *, 30> SectionsToWrite;
   for (const SectionBase &Sec : Obj.allocSections()) {
-    if (Sec.Type != SHT_NOBITS)
+    if (Sec.Type != SHT_NOBITS && Sec.Size > 0)
       SectionsToWrite.push_back(&Sec);
   }
 
diff --git a/llvm/test/tools/llvm-objcopy/ELF/gap-fill.test b/llvm/test/tools/llvm-objcopy/ELF/gap-fill.test
index c11909746330bb..6bfd27924bf244 100644
--- a/llvm/test/tools/llvm-objcopy/ELF/gap-fill.test
+++ b/llvm/test/tools/llvm-objcopy/ELF/gap-fill.test
@@ -106,6 +106,11 @@ Sections:
     Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
     Address:         0x0108
     Content:         'AABBCCDDFEDCBA'
+  - Name:            .zero_size
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    Address:         0x0110
+    Size:            0
   - Name:            .space2
     Type:            Fill
     Pattern:         'DC'

[jh7370]

LGTM, with nits.

[jh7370]

I believe this Size field is unnecessary, although if you want to leave it in for clarity, I won't object.

[quic-akaryaki]

Yes, I'd like to leave it.

[jh7370]

EXECINSTR is irrelevant to the test, right? So let's omit it to avoid confusion.

[quic-akaryaki]

Done

[quic-akaryaki]

Thank you for the feedback @jh7370 .