[lld-macho] Fix use-after-free in loadDylib()

[int3]

Summary

We were taking a reference to a value in loadedDylibs, which in turn
called make<DylibFile>(), which could then recursively call
loadDylibs, which would then potentially resize loadedDylibs and
invalidate that reference.

Fixes PR50101.

[oontvoo]

LGTM - good catch! Is there not any sanitizers build that would've caught this?

[int3]

Is there not any sanitizers build that would've caught this?

It *was* caught by a sanitizer in PR50101. The tricky bit here is that our tests don't trigger a hashmap resize (since the inputs are small), so the ASAN contbuilds didn't catch it.

(Maybe there's value in having a test mode where hashmaps are aggressively resized/rehashed, e.g. on every write. Much like how llvm::sort forces std::sort to be unstable to catch nondeterminism...)

[thakis]

Thanks :)

lld/MachO/DriverUtils.cpp

 
 // It's not uncommon to have multiple attempts to load a single dylib,
 // especially if it's a commonly re-exported core library.
 static DenseMap<CachedHashStringRef, DylibFile *> loadedDylibs;
 
 Optional<DylibFile *> macho::loadDylib(MemoryBufferRef mbref,
                                        DylibFile *umbrella,
                                        bool isBundleLoader) {
-  StringRef path = mbref.getBufferIdentifier();
-  DylibFile *&file = loadedDylibs[CachedHashStringRef(path)];
+  CachedHashStringRef path(mbref.getBufferIdentifier());
+  DylibFile *file = loadedDylibs[path];
   if (file)
     return file;
 
   file_magic magic = identify_magic(mbref.getBuffer());
   if (magic == file_magic::tapi_file) {
     Expected<std::unique_ptr<InterfaceFile>> result = TextAPIReader::get(mbref);
     if (!result) {
       error("could not load TAPI file at " + mbref.getBufferIdentifier() +
             ": " + toString(result.takeError()));
       return {};
     }
     file = make<DylibFile>(**result, umbrella, isBundleLoader);
   } else {
     assert(magic == file_magic::macho_dynamically_linked_shared_lib ||
            magic == file_magic::macho_dynamically_linked_shared_lib_stub ||
            magic == file_magic::macho_executable ||
            magic == file_magic::macho_bundle);
     file = make<DylibFile>(mbref, umbrella, isBundleLoader);
   }
+  // Note that DylibFile's ctor may recursively invoke loadDylib(), which can
+  // cause loadedDylibs to get resized and its iterators invalidated. As such,
+  // we redo the key lookup here instead of caching an iterator from our earlier
+  // lookup at the start of the function.
+  loadedDylibs[path] = file;
   return file;
 }
 
 Optional<InputFile *> macho::loadArchiveMember(MemoryBufferRef mb,
                                                uint32_t modTime,
                                                StringRef archiveName,
                                                bool objCOnly) {
   switch (identify_magic(mb.getBuffer())) {

lld/MachO/InputFiles.h

public:
 
   // An executable can be used as a bundle loader that will load the output
   // file being linked, and that contains symbols referenced, but not
   // implemented in the bundle. When used like this, it is very similar
   // to a Dylib, so we re-used the same class to represent it.
   bool isBundleLoader;
 
 private:
-  template <class LP> void parse(DylibFile *umbrella = nullptr);
+  template <class LP> void parse(DylibFile *umbrella);
 };
 
 // .a file
 class ArchiveFile : public InputFile {
 public:
   explicit ArchiveFile(std::unique_ptr<llvm::object::Archive> &&file);
   static bool classof(const InputFile *f) { return f->kind() == ArchiveKind; }
   void fetch(const llvm::object::Archive::Symbol &sym);