[lld-macho] Add some relocation validation logic

[int3]

Summary

I considered making a Target::validate() method, but I wasn't sure how I felt about the overhead of doing yet another switch-dispatch on the relocation type, so I put the validation in relocateOne instead... might be a bit of a micro-optimization, but relocateOne does assume certain things about the relocations it gets, and this error handling makes that explicit, so it's not a totally unreasonable code organization.

Depends on D80048.

[smeenai]

I'm torn about this one.

One the one hand, as you said, putting the validation in relocateOne is justifiable, since it's making a bunch of assumptions about various relocations. On the other, it feels a bit iffy to be mixing concerns like that, and it also means we won't get errors from broken input files until we start the writing process, which is pretty late.

Would it be possible to do the validation in getImplicitAddend instead? That's at least earlier. It also feels a bit better conceptually ... we could have getImplicitAddend be the function that does any target-specific relocation processing and validation. Unfortunately, I can't think of a good name that conveys that it also computes and returns the implicit addend :/

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

44

CC @alexshap the support you added is coming in handy :)

[int3]

move to getImplicitAddend

[smeenai]

lld/MachO/Arch/X86_64.cpp

28–29

Low-priority follow-up: be consistent about including or not including parameter names in all these declarations, both here and in the header.

45–46

Nit: I feel like "invalid relocation of type X" would read better than "invalid type X relocation", both here and in getImplicitAddend below.

It would be really nice to be able to print the enum as a string instead of just its number (e.g. saying "relocation of type X86_64_RELOC_BRANCH" instead of "relocation of type 2"). There's MachOObjectFile::getRelocationTypeName for this purpose; it's a member function, but we could factor its main logic out into a static function. That definitely shouldn't block this change from going in though; it can easily just be another (low-pri) follow-up.

[smeenai]

Oops, forgot to LGTM. Thanks for the move!

[int3]

lld/MachO/Arch/X86_64.cpp

28–29

my approach has been to omit parameter names when the type is sufficiently descriptive -- which is why the names are present mostly only for parameters with primitive types

[int3]

lld/MachO/Arch/X86_64.cpp

45–46

sounds good. (Also on second thought I don't think we need to put invalid relocation of type X in the prefix; the specific checks will likely always depend on the type and so will mention it in the second part of the error message)

[int3]

address comment

[smeenai]

lld/MachO/Arch/X86_64.cpp

45–46

Looks great!

lld/MachO/Arch/X86_64.cpp

 //===- X86_64.cpp ---------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#include "InputFiles.h"
 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 
 #include "lld/Common/ErrorHandler.h"
 #include "llvm/BinaryFormat/MachO.h"
 #include "llvm/Support/Endian.h"
 
 using namespace llvm::MachO;
 using namespace llvm::support::endian;
 using namespace lld;
 using namespace lld::macho;
 
 namespace {
 
 struct X86_64 : TargetInfo {
   X86_64();
 
-  uint64_t getImplicitAddend(const uint8_t *loc, uint8_t type) const override;
+  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 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
 
-uint64_t X86_64::getImplicitAddend(const uint8_t *loc, uint8_t type) const {
-  switch (type) {
+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();
+}
+
+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:
   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);
   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);
   default:
-    error("TODO: Unhandled relocation type " + std::to_string(type));
+    error("TODO: Unhandled relocation type " + std::to_string(rel.r_type));
     return 0;
   }
 }
 
 void X86_64::relocateOne(uint8_t *loc, uint8_t type, uint64_t val) const {
   switch (type) {
   case X86_64_RELOC_BRANCH:
   case X86_64_RELOC_SIGNED:

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;
-    uint32_t secRelOffset = rel.r_address;
-    uint64_t rawAddend =
-        target->getImplicitAddend(buf + sec.offset + secRelOffset, r.type);
+    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");
 
       if (rel.r_symbolnum == 0 || rel.r_symbolnum > subsections.size())
         fatal("invalid section index in relocation for offset " +
               std::to_string(r.offset) + " in section " + sec.sectname +
               " of " + getName());
 
       SubsectionMap &targetSubsecMap = subsections[rel.r_symbolnum - 1];
       const section_64 &targetSec = sectionHeaders[rel.r_symbolnum - 1];
       // The implicit addend for pcrel section relocations is the pcrel offset
       // in terms of the addresses in the input file. Here we adjust it so that
       // it describes the offset from the start of the target section.
       // TODO: Figure out what to do for non-pcrel section relocations.
       // TODO: The offset of 4 is probably not right for ARM64, nor for
       //       relocations with r_length != 2.
       uint32_t targetOffset =
-          sec.addr + secRelOffset + 4 + rawAddend - targetSec.addr;
+          sec.addr + rel.r_address + 4 + rawAddend - targetSec.addr;
       r.target = findContainingSubsection(targetSubsecMap, &targetOffset);
       r.addend = targetOffset;
     }
 
-    InputSection *subsec = findContainingSubsection(subsecMap, &secRelOffset);
-    r.offset = secRelOffset;
+    r.offset = rel.r_address;
+    InputSection *subsec = findContainingSubsection(subsecMap, &r.offset);
     subsec->relocs.push_back(r);
   }
 }
 
 void InputFile::parseSymbols(ArrayRef<nlist_64> nList, const char *strtab,
                              bool subsectionsViaSymbols) {
   // resize(), not reserve(), because we are going to create N_ALT_ENTRY symbols
   // out-of-sequence.

lld/MachO/Target.h

 //===- Target.h -------------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_MACHO_TARGET_H
 #define LLD_MACHO_TARGET_H
 
+#include "llvm/BinaryFormat/MachO.h"
+#include "llvm/Support/MemoryBuffer.h"
+
 #include <cstddef>
 #include <cstdint>
 
 namespace lld {
 namespace macho {
 
 class DylibSymbol;
+class InputSection;
+struct Reloc;
 
 enum {
   // We are currently only supporting 64-bit targets since macOS and iOS are
   // deprecating 32-bit apps.
   WordSize = 8,
   PageSize = 4096,
   ImageBase = 4096,
   MaxAlignmentPowerOf2 = 32,
 };
 
 class TargetInfo {
 public:
   virtual ~TargetInfo() = default;
 
-  virtual uint64_t getImplicitAddend(const uint8_t *loc,
-                                     uint8_t type) const = 0;
+  // 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;
 
   // 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.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
+#
+# CHECK: error: invalid relocation at offset 1 of __TEXT,__text in [[FILE]]: relocations of type 0 must not be pcrel
+
+!mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x00000003
+  filetype:        0x00000001
+  ncmds:           4
+  sizeofcmds:      280
+  flags:           0x00000000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         ''
+    vmaddr:          0
+    vmsize:          9
+    fileoff:         312
+    filesize:        9
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __text
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        size:            9
+        offset:          0x00000138
+        align:           0
+        reloff:          0x00000144
+        nreloc:          1
+        flags:           0x80000000
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+        content:         '000000000000000000'
+        relocations:
+          - address:         0x00000001
+            symbolnum:       1
+            pcrel:           true
+            length:          3
+            extern:          true
+            type:            0
+            scattered:       false
+            value:           0
+  - cmd:             LC_BUILD_VERSION
+    cmdsize:         24
+    platform:        1
+    minos:           659200
+    sdk:             0
+    ntools:          0
+  - cmd:             LC_SYMTAB
+    cmdsize:         24
+    symoff:          332
+    nsyms:           2
+    stroff:          364
+    strsize:         12
+  - cmd:             LC_DYSYMTAB
+    cmdsize:         80
+    ilocalsym:       0
+    nlocalsym:       0
+    iextdefsym:      0
+    nextdefsym:      2
+    iundefsym:       2
+    nundefsym:       0
+    tocoff:          0
+    ntoc:            0
+    modtaboff:       0
+    nmodtab:         0
+    extrefsymoff:    0
+    nextrefsyms:     0
+    indirectsymoff:  0
+    nindirectsyms:   0
+    extreloff:       0
+    nextrel:         0
+    locreloff:       0
+    nlocrel:         0
+LinkEditData:
+  NameList:
+    - n_strx:          1
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          0
+      n_value:         1
+    - n_strx:          6
+      n_type:          0x0F
+      n_sect:          1
+      n_desc:          0
+      n_value:         9
+  StringTable:
+    - ''
+    - _foo
+    - _main