[COFF] Bounds check relocations

[rnk]

Summary

This would have caught the invalid object file I used in my test case in
r307726. The OOB was only caught by ASan later, which is slow and
doesn't work on some platforms. LLD should do some basic input
validation itself. This check isn't perfect, so relocations can reach
OOB by up to seven bytes, but it's better than what we had and probably
cheap.

[ruiu]

This adds one virtual function call for each relocation. I believe it's negligible, but can you run the linker with and without this patch to see if this is fine?

[grandinj]

lld/COFF/Chunks.cpp

218 ↗

(On Diff #106467)

could hoist getSize() outside the loop?

[rnk]

In D35371#808377, @ruiu wrote:

This adds one virtual function call for each relocation. I believe it's negligible, but can you run the linker with and without this patch to see if this is fine?

Hm, I hadn't noticed that. We know we're dealing with a SectionChunk, so the call doesn't have to be virtual. Maybe we should mark SectionChunk final to get that benefit elsewhere.

lld/COFF/Chunks.cpp

218 ↗

(On Diff #106467)

We should just do that. Even if we inlined the implementation, it's a load.

[rnk]

• Mark SectionChunk final
• Hoist loop-invariant getSize call

[ruiu]

LGTM

lld/trunk/COFF/Chunks.h

public:
   uint64_t OutputSectionOff = 0;
 
 protected:
   // The output section for this chunk.
   OutputSection *Out = nullptr;
 };
 
 // A chunk corresponding a section of an input file.
-class SectionChunk : public Chunk {
+class SectionChunk final : public Chunk {
   // Identical COMDAT Folding feature accesses section internal data.
   friend class ICF;
 
 public:
   class symbol_iterator : public llvm::iterator_adaptor_base<
                               symbol_iterator, const coff_relocation *,
                               std::random_access_iterator_tag, SymbolBody *> {
     friend SectionChunk;

lld/trunk/COFF/Chunks.cpp

 void SectionChunk::writeTo(uint8_t *Buf) const {
   if (!hasData())
     return;
   // Copy section contents from source object file to output file.
   ArrayRef<uint8_t> A = getContents();
   memcpy(Buf + OutputSectionOff, A.data(), A.size());
 
   // Apply relocations.
+  size_t InputSize = getSize();
   for (const coff_relocation &Rel : Relocs) {
+    // Check for an invalid relocation offset. This check isn't perfect, because
+    // we don't have the relocation size, which is only known after checking the
+    // machine and relocation type. As a result, a relocation may overwrite the
+    // beginning of the following input section.
+    if (Rel.VirtualAddress >= InputSize)
+      fatal("relocation points beyond the end of its parent section");
+
     uint8_t *Off = Buf + OutputSectionOff + Rel.VirtualAddress;
 
     // Get the output section of the symbol for this relocation.  The output
     // section is needed to compute SECREL and SECTION relocations used in debug
     // info.
     SymbolBody *Body = File->getSymbolBody(Rel.SymbolTableIndex);
     Defined *Sym = cast<Defined>(Body);
     Chunk *C = Sym->getChunk();

lld/trunk/test/COFF/reloc-oob.yaml

+# Make sure LLD does some light relocation bounds checking.
+
+# RUN: yaml2obj %s -o %t.obj
+# RUN: not lld-link %t.obj -entry:main -nodefaultlib -out:%t.exe 2>&1 | FileCheck %s
+
+# CHECK: error: relocation points beyond the end of its parent section
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_I386
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       16
+    SectionData:     5589E550C745FC00000000A10000000083C4045DC3
+    Relocations:
+      - VirtualAddress:  24
+        SymbolName:      _g
+        Type:            IMAGE_REL_I386_DIR32
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     2A000000
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          21
+      NumberOfRelocations: 1
+      NumberOfLinenumbers: 0
+      CheckSum:        662775349
+      Number:          1
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3482275674
+      Number:          2
+  - Name:            _main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            _g
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...