[lld/mac] Fix issues around thin archives

[thakis]

Summary

• most importantly, fix a use-after-free when using thin archives, by putting the archive unique_ptr to the arena allocator. This ports D65565 to MachO

• correctly demangle symbol namess from archives in diagnostics

• add a test for thin archives -- it finds this UaF, but only when running it under asan (it also finds the demangling fix)

• make forceLoadArchive() use addFile() with a bool to have the archive loading code in fewer places. no behavior change; matches COFF port a bit better

[int3]

lld/MachO/Driver.cpp

323–324

same for the rest of the calls

lld/MachO/Symbols.cpp

22

nit: can we have a blank line between functions? my vim habits depend on it :p

lld/MachO/Symbols.h

238

why not call this toString too? since everything here is MachO anyway

lld/test/MachO/Inputs/mangled-symbols.s

1 ↗

(On Diff #308511)

I'd prefer we use split-file to have this file inline in the main test file. (The tests that currently use Inputs/ were written before we had split-file.)

6–15 ↗

(On Diff #308511)

can we minimize the assembly? I assume CFI directives aren't needed for demangling...

lld/test/MachO/thin-archive.s

14

is running FileCheck really necessary here if the input is going to be empty? I guess the intent is to verify that the CHECK-NOT line doesn't match, but if lld were emitting an error, this line would fail anyway...

might be clearer to just run the commands and check that out contains the symbol definition, similar to what archive.s is doing

[int3]

lld/MachO/Driver.cpp

323–324

actually, I guess this would make for rather verbose code. How about making the parameter default to false instead?

[thakis]

comments

lld/MachO/Driver.cpp

323–324

I think lld code likes to be explicit about arguments -- cf LinkerDriver::enqueuePath() in lld/COFF/Driver.cpp which looks very similar to this.

If you hate the bool I can invent a two-state enum for this so that it's addFile(path, kForceLoadArchive), addFile(path, kRegularFile). But given that there are just a few callers, all in one file, and COFF doesn't do it, I'm not sure it's worth it?

lld/MachO/Symbols.cpp

22

Done. Off-topic: Do you use { / } or [] / ][, [[, ]]? I like the spirit behind the latter but they don't seem to work well for me for C++ code. And { / } movements are often to granular, so I usually navigate with ctrl-f / ctrl-b / search / marks.

lld/MachO/Symbols.h

238

Because the argument is of type llvm::object::Archive::Symbol and we have toXString(llvm::object::Archive::Symbol) in COFF and ELF too (and so normal overloading based on different types in different ports doesn't work). We can't (easily) put toString() in the macho namespace since some of its overloads are in lld/Common. IIRC the change where I added toCOFFString() had some discussion about the various tradeoffs; we settled on not using toString() for the archive symbol.

lld/test/MachO/Inputs/mangled-symbols.s

1 ↗

(On Diff #308511)

Yes, split-file is much nicer, I'm just not used to it. Done :)

6–15 ↗

(On Diff #308511)

Done. The cfi stuff isn't needed; I just pasted the output of clang -S for void f() {}.

[thakis]

missed a comment

lld/test/MachO/thin-archive.s

14

Whoops, missed this. Sure, done.

[int3]

lgtm

lld/MachO/Driver.cpp

323–324

hm fine I guess this is okay as-is

lld/MachO/Symbols.cpp

22

heh yeah I use a lot of { / }, regardless of language

[int3]

lld/MachO/Driver.cpp

323–324

actually, I do see quite a few LLD functions with default arguments... you can see a quick sample via

grep -r " = \"\")"
grep -r " = 0)"

lld/MachO/Driver.cpp

 } // namespace
 
 // Returns slices of MB by parsing MB as an archive file.
 // Each slice consists of a member file in the archive.
 static std::vector<ArchiveMember> getArchiveMembers(MemoryBufferRef mb) {
   std::unique_ptr<Archive> file =
       CHECK(Archive::create(mb),
             mb.getBufferIdentifier() + ": failed to parse archive");
+  Archive *archive = file.get();
+  make<std::unique_ptr<Archive>>(std::move(file)); // take ownership
 
   std::vector<ArchiveMember> v;
   Error err = Error::success();
-  for (const Archive::Child &c : file->children(err)) {
+  for (const Archive::Child &c : archive->children(err)) {
     MemoryBufferRef mbref =
         CHECK(c.getMemoryBufferRef(),
               mb.getBufferIdentifier() +
                   ": could not get the buffer for a child of the archive");
     uint32_t modTime = toTimeT(
         CHECK(c.getLastModified(), mb.getBufferIdentifier() +
                                        ": could not get the modification "
                                        "time for a child of the archive"));
     v.push_back({mbref, modTime});
   }
   if (err)
     fatal(mb.getBufferIdentifier() +
           ": Archive::children failed: " + toString(std::move(err)));
 
   return v;
 }
 
-static void forceLoadArchive(StringRef path) {
-  if (Optional<MemoryBufferRef> buffer = readFile(path)) {
-    for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
-      auto file = make<ObjFile>(member.mbref, member.modTime);
-      file->archiveName = buffer->getBufferIdentifier();
-      inputFiles.push_back(file);
-    }
-  }
-}
-
-static InputFile *addFile(StringRef path) {
+static InputFile *addFile(StringRef path, bool forceLoadArchive) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return nullptr;
   MemoryBufferRef mbref = *buffer;
   InputFile *newFile = nullptr;
 
   switch (identify_magic(mbref.getBuffer())) {
   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(path);
+    if (config->allLoad || forceLoadArchive) {
+      if (Optional<MemoryBufferRef> buffer = readFile(path)) {
+        for (const ArchiveMember &member : getArchiveMembers(*buffer)) {
+          auto file = make<ObjFile>(member.mbref, member.modTime);
+          file->archiveName = buffer->getBufferIdentifier();
+          inputFiles.push_back(file);
+        }
+      }
     } else if (config->forceLoadObjC) {
       for (const object::Archive::Symbol &sym : file->symbols())
         if (sym.getName().startswith(objc::klass))
           symtab->addUndefined(sym.getName());
 
       // 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

 }
 
 static void addFileList(StringRef path) {
   Optional<MemoryBufferRef> buffer = readFile(path);
   if (!buffer)
     return;
   MemoryBufferRef mbref = *buffer;
   for (StringRef path : args::getLines(mbref))
-    addFile(path);
+    addFile(path, false);
 }
 
 static std::array<StringRef, 6> archNames{"arm",    "arm64", "i386",
                                           "x86_64", "ppc",   "ppc64"};
 static bool isArchString(StringRef s) {
   static DenseSet<StringRef> archNamesSet(archNames.begin(), archNames.end());
   return archNamesSet.find(s) != archNamesSet.end();
 }
 
 // An order file has one entry per line, in the following format:
 //
bool macho::link(llvm::ArrayRef<const char *> argsArr, bool canExitEarly,
 
   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());
+      addFile(arg->getValue(), false);
       break;
     case OPT_weak_library: {
-      auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(arg->getValue()));
+      auto *dylibFile =
+          dyn_cast_or_null<DylibFile>(addFile(arg->getValue(), false));
       if (dylibFile)
         dylibFile->forceWeakImport = true;
       break;
     }
     case OPT_filelist:
       addFileList(arg->getValue());
       break;
     case OPT_force_load:
-      forceLoadArchive(arg->getValue());
+      addFile(arg->getValue(), true);
       break;
     case OPT_l:
     case OPT_weak_l: {
       StringRef name = arg->getValue();
       if (Optional<std::string> path = findLibrary(name)) {
-        auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path));
+        auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
         if (opt.getID() == OPT_weak_l && dylibFile)
           dylibFile->forceWeakImport = true;
         break;
       }
       error("library not found for -l" + name);
       break;
     }
     case OPT_framework:
     case OPT_weak_framework: {
       StringRef name = arg->getValue();
       if (Optional<std::string> path = findFramework(name)) {
-        auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path));
+        auto *dylibFile = dyn_cast_or_null<DylibFile>(addFile(*path, false));
         if (opt.getID() == OPT_weak_framework && dylibFile)
           dylibFile->forceWeakImport = true;
         break;
       }
       error("framework not found for -framework " + name);
       break;
     }
     case OPT_platform_version:

lld/MachO/InputFiles.cpp

ArchiveFile::ArchiveFile(std::unique_ptr<llvm::object::Archive> &&f)
   for (const object::Archive::Symbol &sym : file->symbols())
     symtab->addLazy(sym.getName(), this, sym);
 }
 
 void ArchiveFile::fetch(const object::Archive::Symbol &sym) {
   object::Archive::Child c =
       CHECK(sym.getMember(), toString(this) +
                                  ": could not get the member for symbol " +
-                                 sym.getName());
+                                 toMachOString(sym));
 
   if (!seen.insert(c.getChildOffset()).second)
     return;
 
   MemoryBufferRef mb =
       CHECK(c.getMemoryBufferRef(),
             toString(this) +
                 ": could not get the buffer for the member defining symbol " +
-                sym.getName());
+                toMachOString(sym));
 
   uint32_t modTime = toTimeT(
       CHECK(c.getLastModified(), toString(this) +
                                      ": could not get the modification time "
                                      "for the member defining symbol " +
-                                     sym.getName()));
+                                     toMachOString(sym)));
 
   auto file = make<ObjFile>(mb, modTime);
   file->archiveName = getName();
 
   symbols.insert(symbols.end(), file->symbols.begin(), file->symbols.end());
   subsections.insert(subsections.end(), file->subsections.begin(),
                      file->subsections.end());
 }

lld/MachO/Symbols.h

assert(static_cast<Symbol *>(static_cast<T *>(nullptr)) == nullptr &&
          "Not a Symbol");
 
   return new (s) T(std::forward<ArgT>(arg)...);
 }
 
 } // namespace macho
 
 std::string toString(const macho::Symbol &);
+std::string toMachOString(const llvm::object::Archive::Symbol &);
+
 } // namespace lld
 
 #endif

lld/MachO/Symbols.cpp

 //===- Symbols.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 "Symbols.h"
 #include "InputFiles.h"
 #include "SyntheticSections.h"
 
 using namespace llvm;
 using namespace lld;
 using namespace lld::macho;
 
+// Returns a symbol for an error message.
+static std::string demangle(StringRef symName) {
+  if (config->demangle)
+    return demangleItanium(symName);
+  return std::string(symName);
+}
+
+std::string lld::toString(const Symbol &sym) {
+  return demangle(sym.getName());
+}
+
+std::string lld::toMachOString(const object::Archive::Symbol &b) {
+  return demangle(b.getName());
+}
+
 uint64_t Defined::getVA() const {
   if (isAbsolute())
     return value;
   return isec->getVA() + value;
 }
 
 uint64_t Defined::getFileOffset() const {
   if (isAbsolute()) {
     error("absolute symbol " + toString(*this) +
           " does not have a file offset");
     return 0;
   }
   return isec->getFileOffset() + value;
 }
 
 void LazySymbol::fetchArchiveMember() { file->fetch(sym); }
 
-// Returns a symbol for an error message.
-std::string lld::toString(const Symbol &sym) {
-  if (config->demangle)
-    return demangleItanium(sym.getName());
-  return std::string(sym.getName());
-}
-
 uint64_t DSOHandle::getVA() const { return header->addr; }
 
 uint64_t DSOHandle::getFileOffset() const { return header->fileOff; }
 
 constexpr StringRef DSOHandle::name;

lld/test/MachO/thin-archive.s

+# REQUIRES: x86
+
+# RUN: split-file %s %t
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/main.o %t/main.s
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-macos -o %t/lib.o \
+# RUN:     %t/mangled-symbol.s
+# RUN: llvm-ar csr  %t/lib.a      %t/lib.o
+# RUN: llvm-ar csrT %t/lib_thin.a %t/lib.o
+
+# RUN: %lld %t/main.o %t/lib.a -o %t/out
+# RUN: llvm-nm %t/out
+# RUN: %lld %t/main.o %t/lib_thin.a -o %t/out
+# RUN: llvm-nm %t/out
+# RUN: %lld /%t/main.o -force_load %t/lib_thin.a -o %t/out
+# RUN: llvm-nm %t/out
+
+# RUN: rm %t/lib.o
+# RUN: %lld %t/main.o %t/lib.a -o %t/out
+# RUN: llvm-nm %t/out
+# RUN: not %lld %t/main.o %t/lib_thin.a -demangle -o %t/out 2>&1 | \
+# RUN:     FileCheck --check-prefix=NOOBJ %s
+# RUN: not %lld %t/main.o %t/lib_thin.a -o %t/out 2>&1 | \
+# RUN:     FileCheck --check-prefix=NOOBJNODEMANGLE %s
+
+# CHECK: __Z1fv
+# CHECK: _main
+# NOOBJ: error: {{.*}}lib_thin.a: could not get the buffer for the member defining symbol f(): '{{.*}}lib.o':
+# NOOBJNODEMANGLE: error: {{.*}}lib_thin.a: could not get the buffer for the member defining symbol __Z1fv: '{{.*}}lib.o':
+
+#--- mangled-symbol.s
+.globl  __Z1fv
+__Z1fv:
+  retq
+
+#--- main.s
+.global _main
+_main:
+  callq __Z1fv
+  mov $0, %rax
+  retq