[lld-macho] check minimum header length when opening linkable input files

[gkm]

Summary

Bifurcate the readFile() API into ...

• readRawFile() which performs no checks, and
• readLinkableFile() which enforces minimum length of 20 bytes, same as ld64

There are no new tests because tweaks to existing tests are sufficient.

[gkm]

• resubmit to catch parent-diff dependence

[int3]

lld/MachO/Driver.cpp

266

petition to keep this as addFile -- it's not like we have addRawFile, so the additional word seems unnecessary

406

I think the old name made more sense, we are adding the files in the file list, not the filelist file itself

lld/test/MachO/invalid/bad-archive.s

9 ↗

(On Diff #326880)

the original intent of the test was to exercise the archive-parsing code and its error handling, so this is no longer serving its purpose. We could tweak the test, but perhaps it would be easier to just require linkable files to have 4 bytes for the magic number, rather than 20? (plus, I'd like to avoid doing things just because ld64 is doing it, until we find some deeper reason)

similar issue for invalid-fat-narch.s below

lld/test/MachO/rename.s

17

we shouldn't be emitting this message if /dev/null is an output file parameter

[gkm]

• strip "lld: " from FileCheck prefixes BAD1 & BAD2

[gkm]

• add -DAG to FileCheck prefixes BAD1 & BAD2

[gkm]

lld/test/MachO/rename.s

17

The -o was swallowed by -rename_section because of a missing sub arg. Therefore, it is interpreted as an input parameter.

[gkm]

• revise according to review feedback

[int3]

lld/test/MachO/rename.s

17

ah I see. IMO it's kind of weird to test this zero-size-file edge case in rename.s, since it's only tangentially related to renaming. I'd prefer we create a separate test file under test/invalid for it

[int3]

lld/test/MachO/rename.s

17

(also, test seems to be failing on Windows since there isn't a real /dev/null there. Looks like they create a fake temporary file instead)

[gkm]

• remove /dev/null from FileCheck patterns to appease Windoze

[tschuett]

lld/MachO/InputFiles.cpp

124

What are you going to do with. e.g. 4, 5, or 6 bytes files. It only has the magic numbers, but there is no real header?

[gkm]

• add test file lld/test/MachO/invalid/tiny-input.s for degenerate inputs

[gkm]

lld/MachO/InputFiles.cpp

124

Those will be validated downstream, based on magic number. If the magic number doesn't match anything we handle, then it will be rejected as such. If the magic number is valid, then the code for that file type will check that the header & contents are complete & consistent.

[int3]

lld/MachO/InputFiles.cpp

124

I think it's fairly unlikely that we would have a file with a valid magic number but nothing else. We're not trying to defend against fuzzer inputs anyway, as per https://github.com/llvm/llvm-project/blob/main/lld/docs/NewLLD.rst:

The current policy is that it is your responsibility to give trustworthy object files. The function is guaranteed to return as long as you do not pass corrupted or malicious object files. A corrupted file could cause a fatal error or SEGV. That being said, you don't need to worry too much about it if you create object files in the usual way and give them to the linker. It is naturally expected to work, or otherwise it's a linker's bug.

[thakis]

a) I think this breaks --reproduce=repro.tar for -order_file since that previously called readFile() which added to the repro tar, but now readRawFile() doesn't. (Looks like -sectcreate has the same problem likely.)
b) What's the motivation for this change? lld assumes that inputs are produced by a compiler or similar, and are generally valid. What problem is this solving?

[smeenai]

In D97610#2595963, @thakis wrote:

b) What's the motivation for this change? lld assumes that inputs are produced by a compiler or similar, and are generally valid. What problem is this solving?

I believe avoiding ASAN errors, per https://reviews.llvm.org/D97600?id=326855#inline-915154

[gkm]

In D97610#2595963, @thakis wrote:

a) I think this breaks --reproduce=repro.tar for -order_file since that previously called readFile() which added to the repro tar, but now readRawFile() doesn't. (Looks like -sectcreate has the same problem likely.)

-sectcreate is subtle. It should be an opaque object immune to meddling, but as-is, if it happens to be MachO::FAT_MAGIC its arch component will be extracted. I am not going to fret about that until it proves troublesome.

b) What's the motivation for this change? lld assumes that inputs are produced by a compiler or similar, and are generally valid. What problem is this solving?

The immediate motive was to fix an ASAN overflow when checking magic# of an empty buffer. My solution was an ill-conceived refactor with insufficient understanding. Apologies. This reverts the refactor and adds a modest one-line check that the file buffer is long-enough for a magic number: https://reviews.llvm.org/D97757

lld/MachO/Driver.cpp

if (err)
     fatal(mb.getBufferIdentifier() +
           ": Archive::children failed: " + toString(std::move(err)));
 
   return v;
 }
 
 static InputFile *addFile(StringRef path, bool forceLoadArchive,
                           bool isBundleLoader = false) {
-  Optional<MemoryBufferRef> buffer = readFile(path);
+  Optional<MemoryBufferRef> buffer = readLinkableFile(path);
   if (!buffer)
     return nullptr;
   MemoryBufferRef mbref = *buffer;
   InputFile *newFile = nullptr;
 
   auto magic = identify_magic(mbref.getBuffer());
   switch (magic) {
   case file_magic::archive: {
     std::unique_ptr<object::Archive> file = CHECK(
         object::Archive::create(mbref), path + ": failed to parse archive");
 
     if (!file->isEmpty() && !file->hasSymbolTable())
       error(path + ": archive has no index; run ranlib to add one");
 
     if (config->allLoad || forceLoadArchive) {
-      if (Optional<MemoryBufferRef> buffer = readFile(path)) {
+      if (Optional<MemoryBufferRef> buffer = readLinkableFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
           if (Optional<InputFile *> file = loadArchiveMember(
                   member.mbref, member.modTime, path, /*objCOnly=*/false)) {
             inputFiles.insert(*file);
             printArchiveMemberLoad(
                 (forceLoadArchive ? "-force_load" : "-all_load"),
                 inputFiles.back());
           }
         }
       }
     } else if (config->forceLoadObjC) {
       for (const object::Archive::Symbol &sym : file->symbols())
         if (sym.getName().startswith(objc::klass))
           symtab->addUndefined(sym.getName(), /*file=*/nullptr,
                                /*isWeakRef=*/false);
 
       // TODO: no need to look for ObjC sections for a given archive member if
       // we already found that it contains an ObjC symbol. We should also
       // consider creating a LazyObjFile class in order to avoid double-loading
       // these files here and below (as part of the ArchiveFile).
-      if (Optional<MemoryBufferRef> buffer = readFile(path)) {
+      if (Optional<MemoryBufferRef> buffer = readLinkableFile(path)) {
         for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
           if (Optional<InputFile *> file = loadArchiveMember(
                   member.mbref, member.modTime, path, /*objCOnly=*/true)) {
             inputFiles.insert(*file);
             printArchiveMemberLoad("-ObjC", inputFiles.back());
           }
         }
       }
case OPT_framework:
       break;
     default:
       error(arg->getSpelling() + " is not allowed in LC_LINKER_OPTION");
     }
   }
 }
 
 static void addFileList(StringRef path) {
-  Optional<MemoryBufferRef> buffer = readFile(path);
+  Optional<MemoryBufferRef> buffer = readRawFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
     addFile(path, false);
 }
 
 // An order file has one entry per line, in the following format:
 //
 //   <cpu>:<object file>:<symbol name>
 //
 // <cpu> and <object file> are optional. If not specified, then that entry
 // matches any symbol of that name. Parsing this format is not quite
 // straightforward because the symbol name itself can contain colons, so when
 // encountering a colon, we consider the preceding characters to decide if it
 // can be a valid CPU type or file path.
 //
 // If a symbol is matched by multiple entries, then it takes the lowest-ordered
 // entry (the one nearest to the front of the list.)
 //
 // The file can also have line comments that start with '#'.
 static void parseOrderFile(StringRef path) {
-  Optional<MemoryBufferRef> buffer = readFile(path);
+  Optional<MemoryBufferRef> buffer = readRawFile(path);
   if (!buffer) {
     error("Could not read order file at " + path);
     return;
   }
 
   MemoryBufferRef mbref = *buffer;
   size_t priority = std::numeric_limits<size_t>::max();
   for (StringRef line : args::getLines(mbref)) {
bool macho::link(ArrayRef<const char *> argsArr, bool canExitEarly,
 
   createSyntheticSections();
   symtab->addDSOHandle(in.header);
 
   for (opt::Arg *arg : args.filtered(OPT_sectcreate)) {
     StringRef segName = arg->getValue(0);
     StringRef sectName = arg->getValue(1);
     StringRef fileName = arg->getValue(2);
-    Optional<MemoryBufferRef> buffer = readFile(fileName);
+    Optional<MemoryBufferRef> buffer = readRawFile(fileName);
     if (buffer)
       inputFiles.insert(make<OpaqueFile>(*buffer, segName, sectName));
   }
 
   // Initialize InputSections.
   for (InputFile *file : inputFiles) {
     for (SubsectionMap &map : file->subsections) {
       for (auto &p : map) {

lld/MachO/DriverUtils.cpp

case OPT_reproduce:
       break;
     case OPT_INPUT:
       os << quote(rewritePath(arg->getValue())) << "\n";
       break;
     case OPT_o:
       os << "-o " << quote(path::filename(arg->getValue())) << "\n";
       break;
     case OPT_filelist:
-      if (Optional<MemoryBufferRef> buffer = readFile(arg->getValue()))
+      if (Optional<MemoryBufferRef> buffer = readRawFile(arg->getValue()))
         for (StringRef path : args::getLines(*buffer))
           os << quote(rewritePath(path)) << "\n";
       break;
     case OPT_force_load:
     case OPT_rpath:
     case OPT_syslibroot:
     case OPT_F:
     case OPT_L:

lld/MachO/InputFiles.h

public:
   explicit BitcodeFile(MemoryBufferRef mb);
   static bool classof(const InputFile *f) { return f->kind() == BitcodeKind; }
 
   std::unique_ptr<llvm::lto::InputFile> obj;
 };
 
 extern llvm::SetVector<InputFile *> inputFiles;
 
-llvm::Optional<MemoryBufferRef> readFile(StringRef path);
+llvm::Optional<MemoryBufferRef> readRawFile(StringRef path);
+llvm::Optional<MemoryBufferRef> readLinkableFile(StringRef path);
 
 const llvm::MachO::load_command *
 findCommand(const llvm::MachO::mach_header_64 *, uint32_t type);
 
 } // namespace macho
 
 std::string toString(const macho::InputFile *file);
 } // namespace lld
 
 #endif

lld/MachO/InputFiles.cpp

return (path::filename(f->archiveName) + "(" + path::filename(f->getName()) +
       .str();
 }
 
 SetVector<InputFile *> macho::inputFiles;
 std::unique_ptr<TarWriter> macho::tar;
 int InputFile::idCount = 0;
 
 // Open a given file path and return it as a memory-mapped file.
-Optional<MemoryBufferRef> macho::readFile(StringRef path) {
+// Perform no sanity checks--just open, map & return.
+Optional<MemoryBufferRef> macho::readRawFile(StringRef path) {
   // Open a file.
   auto mbOrErr = MemoryBuffer::getFile(path);
   if (auto ec = mbOrErr.getError()) {
     error("cannot open " + path + ": " + ec.message());
     return None;
   }
 
   std::unique_ptr<MemoryBuffer> &mb = *mbOrErr;
   MemoryBufferRef mbref = mb->getMemBufferRef();
   make<std::unique_ptr<MemoryBuffer>>(std::move(mb)); // take mb ownership
+  return mbref;
+}
+
+// Open a given file path and return it as a memory-mapped file.
+// Assume the file has one of a variety of linkable formats and
+// perform some basic sanity checks, notably minimum length.
+Optional<MemoryBufferRef> macho::readLinkableFile(StringRef path) {
+  Optional<MemoryBufferRef> maybeMbref = readRawFile(path);
+  if (!maybeMbref) {
+    return None;
+  }
+  MemoryBufferRef mbref = *maybeMbref;
+
+  // LD64 hard-codes 20 as minimum header size, which is presumably
+  // the smallest header among the the various linkable input formats
+  // LLD are less demanding. We insist on having only enough data for
+  // a magic number.
+  if (mbref.getBufferSize() < sizeof(uint32_t)) {
+    error("file is too small to contain a magic number: " + path);
+    return None;
+  }
 
   // If this is a regular non-fat file, return it.
   const char *buf = mbref.getBufferStart();
   auto *hdr = reinterpret_cast<const MachO::fat_header *>(buf);
   if (read32be(&hdr->magic) != MachO::FAT_MAGIC) {
     if (tar)
       tar->append(relativeToRoot(path), mbref.getBuffer());
     return mbref;
void ObjFile::parseDebugInfo() {
   const DWARFContext::compile_unit_range &units = ctx->compile_units();
   auto it = units.begin();
   compileUnit = it->get();
   assert(std::next(it) == units.end());
 }
 
 // The path can point to either a dylib or a .tbd file.
 static Optional<DylibFile *> loadDylib(StringRef path, DylibFile *umbrella) {
-  Optional<MemoryBufferRef> mbref = readFile(path);
+  Optional<MemoryBufferRef> mbref = readLinkableFile(path);
   if (!mbref) {
     error("could not read dylib file at " + path);
     return {};
   }
   return loadDylib(*mbref, umbrella);
 }
 
 // TBD files are parsed into a series of TAPI documents (InterfaceFiles), with

lld/test/MachO/invalid/tiny-input.s

+# REQUIRES: x86
+
+## Check that files too short to have a magic number are rejected as inputs
+# RUN: echo -n 1 >%t-1.o
+# RUN: echo -n 12 >%t-2.o
+# RUN: echo -n 123 >%t-3.o
+# RUN: echo -n 1234 >%t-4.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
+# RUN: not %lld -o %t %t.o %t-1.o %t-2.o %t-3.o %t-4.o 2>&1 | FileCheck %s
+
+# CHECK: error: file is too small to contain a magic number: {{.*}}-1.o
+# CHECK: error: file is too small to contain a magic number: {{.*}}-2.o
+# CHECK: error: file is too small to contain a magic number: {{.*}}-3.o
+# CHECK: error: {{.*}}-4.o: unhandled file type
+
+.global _main
+_main:
+  ret

lld/test/MachO/rename.s

 # REQUIRES: x86
 # RUN: rm -fr %t
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
 # RUN: %lld -o %t %t.o
 
 ## Check option format
 # RUN: not %lld \
 # RUN:     -rename_section B@GUS_SEG b@gus_sect S/ASHY_SEG st*rry_sect \
 # RUN:     -rename_section __FROM_SECT __from_sect __TO_SECT \
 # RUN:   -o /dev/null %t.o 2>&1 | FileCheck %s --check-prefix=BAD1
 
 # BAD1-DAG: error: invalid name for segment or section: B@GUS_SEG
 # BAD1-DAG: error: invalid name for segment or section: b@gus_sect
 # BAD1-DAG: error: invalid name for segment or section: S/ASHY_SEG
 # BAD1-DAG: error: invalid name for segment or section: st*rry_sect
 # BAD1-DAG: error: invalid name for segment or section: -o
-# BAD1-DAG: error: /dev/null: unhandled file type
+# BAD1-DAG: error: file is too small to contain a magic number:
 
 # RUN: not %lld \
 # RUN:     -rename_segment H#SHY_SEG PL+SSY_SEG \
 # RUN:     -rename_segment __FROM_SEG \
 # RUN:   -o /dev/null %t.o 2>&1 | FileCheck %s --check-prefix=BAD2
 
 # BAD2-DAG: error: invalid name for segment or section: H#SHY_SEG
 # BAD2-DAG: error: invalid name for segment or section: PL+SSY_SEG
 # BAD2-DAG: error: invalid name for segment or section: -o
-# BAD2-DAG: error: /dev/null: unhandled file type
+# BAD2-DAG: error: file is too small to contain a magic number:
 
 ## Check that section and segment renames happen
 # RUN: %lld \
 # RUN:     -rename_section __FROM_SECT __from_sect __TO_SECT __to_sect \
 # RUN:     -rename_segment __FROM_SEG __TO_SEG \
 # RUN:   -o %t %t.o
 # RUN: llvm-objdump --macho --all-headers %t | FileCheck %s