[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

264–266

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

407–408

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

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

static std::vector<ArchiveMember> getArchiveMembers(MemoryBufferRef mb) {
   }
   if (err)
     fatal(mb.getBufferIdentifier() +
           ": Archive::children failed: " + toString(std::move(err)));
 
   return v;
 }
 
-static InputFile *addFile(StringRef path, bool forceLoadArchive,
+static InputFile *addLinkableFile(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());
           }
         }
       }
if (config->printEachFile && magic != file_magic::archive)
       message(toString(newFile));
     inputFiles.insert(newFile);
   }
   return newFile;
 }
 
 static void addLibrary(StringRef name, bool isWeak) {
   if (Optional<std::string> path = findLibrary(name)) {
-    auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
+    auto *dylibFile =
+        dyn_cast_or_null<DylibFile>(addLinkableFile(*path, false));
     if (isWeak && dylibFile)
       dylibFile->forceWeakImport = true;
     return;
   }
   error("library not found for -l" + name);
 }
 
 static void addFramework(StringRef name, bool isWeak) {
   if (Optional<std::string> path = findFramework(name)) {
-    auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
+    auto *dylibFile =
+        dyn_cast_or_null<DylibFile>(addLinkableFile(*path, false));
     if (isWeak && dylibFile)
       dylibFile->forceWeakImport = true;
     return;
   }
   error("framework not found for -framework " + name);
 }
 
 // Parses LC_LINKER_OPTION contents, which can add additional command line flags.
case OPT_framework:
       addFramework(arg->getValue(), false);
       break;
     default:
       error(arg->getSpelling() + " is not allowed in LC_LINKER_OPTION");
     }
   }
 }
 
-static void addFileList(StringRef path) {
-  Optional<MemoryBufferRef> buffer = readFile(path);
+static void addFileListFile(StringRef path) {
+  Optional<MemoryBufferRef> buffer = readRawFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(path, false);
+    addLinkableFile(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,
   config->headerPadMaxInstallNames =
       args.hasArg(OPT_headerpad_max_install_names);
   config->printEachFile = args.hasArg(OPT_t);
   config->printWhyLoad = args.hasArg(OPT_why_load);
   config->outputType = getOutputType(args);
   if (const opt::Arg *arg = args.getLastArg(OPT_bundle_loader)) {
     if (config->outputType != MH_BUNDLE)
       error("-bundle_loader can only be used with MachO bundle output");
-    addFile(arg->getValue(), false, true);
+    addLinkableFile(arg->getValue(), false, true);
   }
   config->ltoObjPath = args.getLastArgValue(OPT_object_path_lto);
   config->ltoNewPassManager =
       args.hasFlag(OPT_no_lto_legacy_pass_manager, OPT_lto_legacy_pass_manager,
                    LLVM_ENABLE_NEW_PASS_MANAGER);
   config->runtimePaths = args::getStrings(args, OPT_rpath);
   config->allLoad = args.hasArg(OPT_all_load);
   config->forceLoadObjC = args.hasArg(OPT_ObjC);
if (args.hasArg(OPT_v)) {
     message(StringRef("Framework search paths:") +
             (config->frameworkSearchPaths.size()
                  ? "\n\t" + join(config->frameworkSearchPaths, "\n\t")
                  : ""));
     freeArena();
     return !errorCount();
   }
 
-  initLLVM(); // must be run before any call to addFile()
+  initLLVM(); // must be run before any call to addLinkableFile()
 
   for (const auto &arg : args) {
     const auto &opt = arg->getOption();
     warnIfDeprecatedOption(opt);
     warnIfUnimplementedOption(opt);
 
     // TODO: are any of these better handled via filtered() or getLastArg()?
     switch (opt.getID()) {
     case OPT_INPUT:
-      addFile(arg->getValue(), false);
+      addLinkableFile(arg->getValue(), false);
       break;
     case OPT_weak_library:
-      if (auto *dylibFile =
-              dyn_cast_or_null<DylibFile>(addFile(arg->getValue(), false)))
+      if (auto *dylibFile = dyn_cast_or_null<DylibFile>(
+              addLinkableFile(arg->getValue(), false)))
         dylibFile->forceWeakImport = true;
       break;
     case OPT_filelist:
-      addFileList(arg->getValue());
+      addFileListFile(arg->getValue());
       break;
     case OPT_force_load:
-      addFile(arg->getValue(), true);
+      addLinkableFile(arg->getValue(), true);
       break;
     case OPT_l:
     case OPT_weak_l:
       addLibrary(arg->getValue(), opt.getID() == OPT_weak_l);
       break;
     case OPT_framework:
     case OPT_weak_framework:
       addFramework(arg->getValue(), opt.getID() == OPT_weak_framework);
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
+  const size_t minimumLinkableFileHeaderSize = 20;
+  if (mbref.getBufferSize() < minimumLinkableFileHeaderSize) {
+    error("file is too small to contain a linkable-file header: " + 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/bad-archive.s

 # REQUIRES: x86
 # RUN: echo "!<arch>" > %t.a
 # RUN: echo "foo" >> %t.a
 # RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t.o
 
 # RUN: not %lld %t.o %t.a -o /dev/null 2>&1 | FileCheck -DFILE=%t.a %s
 # RUN: not %lld %t.o -force_load %t.a -o /dev/null 2>&1 | FileCheck -DFILE=%t.a %s
 # RUN: not %lld %t.o -ObjC %t.a -o /dev/null 2>&1 | FileCheck -DFILE=%t.a %s
-# CHECK: error: [[FILE]]: failed to parse archive: truncated or malformed archive (remaining size of archive too small for next archive member header at offset 8)
+# CHECK: error: file is too small to contain a linkable-file header: [[FILE]]
 
 .global _main
 _main:
   ret

lld/test/MachO/invalid/invalid-fat-narch.s

 # REQUIRES: x86
 # RUN: yaml2obj %s -o %t.o
 # RUN: not %lld -o /dev/null %t.o 2>&1 | \
 # RUN:    FileCheck %s -DFILE=%t.o
-# CHECK: error: [[FILE]]: fat_arch struct extends beyond end of file
+# CHECK: error: file is too small to contain a linkable-file header: [[FILE]]
 
 !fat-mach-o
 FatHeader:
   magic:           0xCAFEBABE
   nfat_arch:       2
 FatArchs:
 Slices:

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
+# RUN:   -o /dev/null %t.o 2>&1 | FileCheck %s --check-prefix=BAD1
+
+# BAD1: error: invalid name for segment or section: S/ASHY_SEG
+# BAD1: error: invalid name for segment or section: st*rry_sect
+# BAD1: error: invalid name for segment or section: B@GUS_SEG
+# BAD1: error: invalid name for segment or section: b@gus_sect
+# BAD1: error: invalid name for segment or section: -o
+# BAD1: error: file is too small to contain a linkable-file header: /dev/null
+
 # RUN: not %lld \
 # RUN:     -rename_segment H#SHY_SEG pl+ssy_sect \
 # RUN:     -rename_segment __FROM_SEG \
-# RUN:   -o /dev/null %t.o
+# RUN:   -o /dev/null %t.o 2>&1 | FileCheck %s --check-prefix=BAD2
+
+# BAD2: error: invalid name for segment or section: pl+ssy_sect
+# BAD2: error: invalid name for segment or section: H#SHY_SEG
+# BAD2: error: invalid name for segment or section: -o
+# BAD2: error: file is too small to contain a linkable-file header: /dev/null
 
 ## 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