[lld-macho] Properly handle & validate relocation r_length

[int3]

Summary

We should be reading / writing our addends / relocated addresses based on
r_length, and not just based on the type of the relocation. But since only
some r_length values are valid for a given reloc type, I've also added some
validation.

ld64 has code to allow for r_length = 0 in X86_64_RELOC_BRANCH relocs, but I'm
not sure how to create such a relocation...

Depends on D80414.

[int3]

update

[smeenai]

This is great!

lld/MachO/Arch/X86_64.cpp

58

It's an error path so efficiency doesn't matter, but in general, Twine is a better choice for concatenations like this.

76

It might be better to disallow the 0 case for now, and comment that we're diverging from ld64's behavior there. That way we'll get a loud error if we do end up seeing that case in the wild, and we can figure out what's going on with it then :)

Short jumps are a thing (where the offset is a single byte), but I can't imagine when we'd emit a relocation for one :/

124–126

In particular, this is incorrect if we're allowing the r_length for an X86_64_RELOC_BRANCH to be 0.

[int3]

lld/MachO/Arch/X86_64.cpp

58

hm I spent some time trying to use Twine but ended up with all sorts of corrupted strings. I even put all the intermediate temp values into local variables, but still no dice...

124–126

Good catch! Added to the comment.

[int3]

address comments

lld/MachO/Arch/X86_64.cpp

 
 namespace {
 
 struct X86_64 : TargetInfo {
   X86_64();
 
   uint64_t getImplicitAddend(MemoryBufferRef, const section_64 &,
                              const relocation_info &) const override;
-  void relocateOne(uint8_t *loc, uint8_t type, uint64_t val) const override;
+  void relocateOne(uint8_t *loc, const Reloc &, uint64_t val) const override;
 
   void writeStub(uint8_t *buf, const DylibSymbol &) const override;
   void writeStubHelperHeader(uint8_t *buf) const override;
   void writeStubHelperEntry(uint8_t *buf, const DylibSymbol &,
                             uint64_t entryAddr) const override;
 
   void prepareDylibSymbolRelocation(DylibSymbol &, uint8_t type) override;
   uint64_t getDylibSymbolVA(const DylibSymbol &, uint8_t type) const override;
 };
 
 } // namespace
 
 static std::string getErrorLocation(MemoryBufferRef mb, const section_64 &sec,
                                     const relocation_info &rel) {
   return ("invalid relocation at offset " + std::to_string(rel.r_address) +
           " of " + sec.segname + "," + sec.sectname + " in " +
           mb.getBufferIdentifier())
       .str();
 }
 
+static void validateLength(MemoryBufferRef mb, const section_64 &sec,
+                           const relocation_info &rel,
+                           const std::vector<uint8_t> &validLengths) {
+  if (std::find(validLengths.begin(), validLengths.end(), rel.r_length) !=
+      validLengths.end())
+    return;
+
+  std::string msg = getErrorLocation(mb, sec, rel) + ": relocations of type " +
+                    std::to_string(rel.r_type) + " must have r_length of ";
+  bool first = true;
+  for (uint8_t length : validLengths) {
+    if (!first)
+      msg += " or ";
+    first = false;
+    msg += std::to_string(length);
+  }
+  fatal(msg);
+}
+
 uint64_t X86_64::getImplicitAddend(MemoryBufferRef mb, const section_64 &sec,
                                    const relocation_info &rel) const {
   auto *buf = reinterpret_cast<const uint8_t *>(mb.getBufferStart());
   const uint8_t *loc = buf + sec.offset + rel.r_address;
   switch (rel.r_type) {
   case X86_64_RELOC_BRANCH:
+    // XXX: ld64 also supports r_length = 0 here but I'm not sure when such a
+    // relocation will actually be generated.
+    validateLength(mb, sec, rel, {2});
+    break;
   case X86_64_RELOC_SIGNED:
   case X86_64_RELOC_SIGNED_1:
   case X86_64_RELOC_SIGNED_2:
   case X86_64_RELOC_SIGNED_4:
   case X86_64_RELOC_GOT_LOAD:
     if (!rel.r_pcrel)
       fatal(getErrorLocation(mb, sec, rel) + ": relocations of type " +
             std::to_string(rel.r_type) + " must be pcrel");
-    return read32le(loc);
+    validateLength(mb, sec, rel, {2});
+    break;
   case X86_64_RELOC_UNSIGNED:
     if (rel.r_pcrel)
       fatal(getErrorLocation(mb, sec, rel) + ": relocations of type " +
             std::to_string(rel.r_type) + " must not be pcrel");
-    return read64le(loc);
+    validateLength(mb, sec, rel, {2, 3});
+    break;
   default:
     error("TODO: Unhandled relocation type " + std::to_string(rel.r_type));
     return 0;
   }
+
+  switch (rel.r_length) {
+  case 0:
+    return *loc;
+  case 1:
+    return read16le(loc);
+  case 2:
+    return read32le(loc);
+  case 3:
+    return read64le(loc);
+  default:
+    llvm_unreachable("invalid r_length");
+  }
 }
 
-void X86_64::relocateOne(uint8_t *loc, uint8_t type, uint64_t val) const {
-  switch (type) {
+void X86_64::relocateOne(uint8_t *loc, const Reloc &r, uint64_t val) const {
+  switch (r.type) {
   case X86_64_RELOC_BRANCH:
   case X86_64_RELOC_SIGNED:
   case X86_64_RELOC_SIGNED_1:
   case X86_64_RELOC_SIGNED_2:
   case X86_64_RELOC_SIGNED_4:
   case X86_64_RELOC_GOT_LOAD:
     // These types are only used for pc-relative relocations, so offset by 4
-    // since the RIP has advanced by 4 at this point.
-    write32le(loc, val - 4);
+    // since the RIP has advanced by 4 at this point. This is only valid when
+    // r_length = 0, which is enforced by validateLength().
+    val -= 4;
     break;
   case X86_64_RELOC_UNSIGNED:
-    write64le(loc, val);
     break;
   default:
     llvm_unreachable(
         "getImplicitAddend should have flagged all unhandled relocation types");
   }
+
+  switch (r.length) {
+  case 0:
+    *loc = val;
+    break;
+  case 1:
+    write16le(loc, val);
+    break;
+  case 2:
+    write32le(loc, val);
+    break;
+  case 3:
+    write64le(loc, val);
+    break;
+  default:
+    llvm_unreachable("invalid r_length");
+  }
 }
 
 // The following methods emit a number of assembly sequences with RIP-relative
 // addressing. Note that RIP-relative addressing on X86-64 has the RIP pointing
 // to the next instruction, not the current instruction, so we always have to
 // account for the current instruction's size when calculating offsets.
 // writeRipRelative helps with that.
 //

lld/MachO/InputFiles.cpp

for (const any_relocation_info &anyRel : relInfos) {
     if (anyRel.r_word0 & R_SCATTERED)
       fatal("TODO: Scattered relocations not supported");
 
     auto rel = reinterpret_cast<const relocation_info &>(anyRel);
 
     Reloc r;
     r.type = rel.r_type;
     r.pcrel = rel.r_pcrel;
+    r.length = rel.r_length;
     uint64_t rawAddend = target->getImplicitAddend(mb, sec, rel);
 
     if (rel.r_extern) {
       r.target = symbols[rel.r_symbolnum];
       r.addend = rawAddend;
     } else {
       if (!rel.r_pcrel)
         fatal("TODO: Only pcrel section relocations are supported");

lld/MachO/InputSection.h

 class InputFile;
 class InputSection;
 class OutputSection;
 class Symbol;
 
 struct Reloc {
   uint8_t type;
   bool pcrel;
+  uint8_t length;
   // The offset from the start of the subsection that this relocation belongs
   // to.
   uint32_t offset;
   // Adding this offset to the address of the target symbol or subsection gives
   // the destination that this relocation refers to.
   uint64_t addend;
   llvm::PointerUnion<Symbol *, InputSection *> target;
 };

lld/MachO/InputSection.cpp

if (auto *s = r.target.dyn_cast<Symbol *>()) {
       }
     } else if (auto *isec = r.target.dyn_cast<InputSection *>()) {
       va = isec->getVA();
     }
 
     uint64_t val = va + r.addend;
     if (r.pcrel)
       val -= getVA() + r.offset;
-    target->relocateOne(buf + r.offset, r.type, val);
+    target->relocateOne(buf + r.offset, r, val);
   }
 }

lld/MachO/Target.h

 class TargetInfo {
 public:
   virtual ~TargetInfo() = default;
 
   // Validate the relocation structure and get its addend.
   virtual uint64_t
   getImplicitAddend(llvm::MemoryBufferRef, const llvm::MachO::section_64 &,
                     const llvm::MachO::relocation_info &) const = 0;
-  virtual void relocateOne(uint8_t *loc, uint8_t type, uint64_t val) const = 0;
+  virtual void relocateOne(uint8_t *loc, const Reloc &, uint64_t val) const = 0;
 
   // Write code for lazy binding. See the comments on StubsSection for more
   // details.
   virtual void writeStub(uint8_t *buf, const DylibSymbol &) const = 0;
   virtual void writeStubHelperHeader(uint8_t *buf) const = 0;
   virtual void writeStubHelperEntry(uint8_t *buf, const DylibSymbol &,
                                     uint64_t entryAddr) const = 0;
 

lld/test/MachO/invalid/invalid-relocation-length.yaml

 # REQUIRES: x86
 # RUN: yaml2obj %s -o %t.o
-# RUN: not lld -flavor darwinnew -o %t %t.o 2>&1 | FileCheck %s -DFILE=%t.o
+# RUN: not lld -flavor darwinnew -arch x86_64 -o %t %t.o 2>&1 | FileCheck %s -DFILE=%t.o
 #
-# CHECK: error: invalid relocation at offset 1 of __TEXT,__text in [[FILE]]: relocations of type 0 must not be pcrel
+# CHECK: error: invalid relocation at offset 1 of __TEXT,__text in [[FILE]]: relocations of type 0 must have r_length of 2 or 3
 
 !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x00000003
   filetype:        0x00000001
   ncmds:           4
Sections:
         flags:           0x80000000
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
         content:         '000000000000000000'
         relocations:
           - address:         0x00000001
             symbolnum:       1
-            pcrel:           true
-            length:          3
+            pcrel:           false
+            length:          1
             extern:          true
             type:            0
             scattered:       false
             value:           0
   - cmd:             LC_BUILD_VERSION
     cmdsize:         24
     platform:        1
     minos:           659200

lld/test/MachO/x86-64-reloc-unsigned.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
 # RUN: lld -flavor darwinnew -o %t %t.o
 # RUN: llvm-objdump --full-contents %t | FileCheck %s
 # CHECK: Contents of section foo:
 # CHECK:  100001000 08100000 01000000
 # CHECK: Contents of section bar:
-# CHECK:  100001008 11211111 02000000
+# CHECK:  100001008 011000f0 11211111 02000000
 
 .globl _main, _foo, _bar
 
 .section __DATA,foo
 _foo:
 .quad _bar
 
 .section __DATA,bar
 _bar:
-## The unsigned relocation should support 64-bit addends
+## We create a .int symbol reference here -- with non-zero data immediately
+## after -- to check that lld reads precisely 32 bits (and not more) of the
+## implicit addend when handling unsigned relocations of r_length = 2.
+## Note that __PAGEZERO occupies the lower 32 bits, so all symbols are above
+## that. To get a final relocated address that fits within 32 bits, we need to
+## subtract an offset here.
+.int _foo - 0x0fffffff
+## The unsigned relocation should support 64-bit addends too (r_length = 3).
 .quad _foo + 0x111111111
 
 .text
 _main:
   mov $0, %rax
   ret