[lld-macho] Rework length check when opening input files

[gkm]

Summary

This reverts diff D97610 (commit 0223ab035c199e537a0040857ba147ced87fd533) and adds a one-line fix to verify that a MemoryBufferRef has sufficient length before reading a 4-byte magic number.

[gkm]

lld/MachO/InputFiles.cpp

109

This is the new guard for adequate buffer size before reading the magic#

[thakis]

Thanks, much nicer :)

It's important to keep in mind that 99.9% of lld invocations (on non-Win) are through clang, and clang will never pass /dev/null as an input file (unless you try really hard, with -Wl,/dev/null. So I don't even think crashing on this input would be all that terrible. But if it can be prevented with a simple one-line fix, then fixing is fine too I suppose.

[thakis]

lld/test/MachO/rename.s

17

Looks like on Windows /dev/null is replaced with a temp file:

ld64.lld: error: C:\Users\ContainerAdministrator\AppData\Local\Temp\lit_tmp_xrue9d5c\tmprj2ybz87: unhandled file type

So probably use {{.*}} as file name here and below,

[gkm]

• For Windows's benefit: s//dev/null/{{.*}}/ in tests

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 = readLinkableFile(path);
+  Optional<MemoryBufferRef> buffer = readFile(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 = readLinkableFile(path)) {
+      if (Optional<MemoryBufferRef> buffer = readFile(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 = readLinkableFile(path)) {
+      if (Optional<MemoryBufferRef> buffer = readFile(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 = readRawFile(path);
+  Optional<MemoryBufferRef> buffer = readFile(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 = readRawFile(path);
+  Optional<MemoryBufferRef> buffer = readFile(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 = readRawFile(fileName);
+    Optional<MemoryBufferRef> buffer = readFile(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 = readRawFile(arg->getValue()))
+      if (Optional<MemoryBufferRef> buffer = readFile(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> readRawFile(StringRef path);
-llvm::Optional<MemoryBufferRef> readLinkableFile(StringRef path);
+llvm::Optional<MemoryBufferRef> readFile(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.
-// Perform no sanity checks--just open, map & return.
-Optional<MemoryBufferRef> macho::readRawFile(StringRef path) {
+Optional<MemoryBufferRef> macho::readFile(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 (mbref.getBufferSize() < sizeof(uint32_t) ||
+      read32be(&hdr->magic) != MachO::FAT_MAGIC) {
     if (tar)
       tar->append(relativeToRoot(path), mbref.getBuffer());
     return mbref;
   }
 
   // Object files and archive files may be fat files, which contains
   // multiple real files for different CPU ISAs. Here, we search for a
   // file that matches with the current link target and returns it as
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 = readLinkableFile(path);
+  Optional<MemoryBufferRef> mbref = readFile(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: file is too small to contain a magic number:
+# BAD1-DAG: error: /dev/null: unhandled file type
 
 # 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: file is too small to contain a magic number:
+# BAD2-DAG: error: /dev/null: unhandled file type
 
 ## 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