[dsymutil] Fix memory issue in the BinaryHolder

[JDevlieghere]

Summary

The BinaryHolder has two caches for object and archive entries. These are implemented as StringMaps of ObjectEntry and ArchiveEntry respectively. The fact that they're stored by value is problematic because the BinaryHolder hands out references that become invalidate when the data structure grows. This patch wraps those object instances in unique pointers and changes the interface to hand out pointers. This resulted in transient failures

[vsk]

llvm/tools/dsymutil/BinaryHolder.cpp

227–228

I think we can end up with two distinct ObjectEntry's (at different addresses) for the same {filename, timestamp} key, because archive loading is racy. Is that ok?

[JDevlieghere]

llvm/tools/dsymutil/BinaryHolder.cpp

227–228

That's a good point. I guess there's no better way to avoid that than by locking the during the whole loading sequence.

[JDevlieghere]

Lock during the whole load sequence

[vsk]

llvm/tools/dsymutil/BinaryHolder.cpp

227–228

I can't think of a better option.

[vsk]

Thanks, lgtm.

[vsk]

llvm/tools/dsymutil/BinaryHolder.cpp

2

?

[vsk]

llvm/tools/dsymutil/BinaryHolder.cpp

179

This comment might be stale now.

llvm/tools/dsymutil/BinaryHolder.h

Error load(IntrusiveRefCntPtr<vfs::FileSystem> VFS, StringRef Filename,
                TimestampTy Timestamp, bool Verbose = false);
 
     Expected<const ObjectEntry &> getObjectEntry(StringRef Filename,
                                                  TimestampTy Timestamp,
                                                  bool Verbose = false);
 
   private:
     std::vector<std::unique_ptr<object::Archive>> Archives;
-    DenseMap<KeyTy, ObjectEntry> MemberCache;
+    DenseMap<KeyTy, std::unique_ptr<ObjectEntry>> MemberCache;
     std::mutex MemberCacheMutex;
   };
 
   Expected<const ObjectEntry &>
   getObjectEntry(StringRef Filename, TimestampTy Timestamp = TimestampTy());
 
   void clear();
 
 private:
   /// Cache of static archives. Objects that are part of a static archive are
   /// stored under this object, rather than in the map below.
-  StringMap<ArchiveEntry> ArchiveCache;
+  StringMap<std::unique_ptr<ArchiveEntry>> ArchiveCache;
   std::mutex ArchiveCacheMutex;
 
   /// Object entries for objects that are not in a static archive.
-  StringMap<ObjectEntry> ObjectCache;
+  StringMap<std::unique_ptr<ObjectEntry>> ObjectCache;
   std::mutex ObjectCacheMutex;
 
   /// Virtual File System instance.
   IntrusiveRefCntPtr<vfs::FileSystem> VFS;
 
   bool Verbose;
 };
 

llvm/tools/dsymutil/BinaryHolder.cpp

 //===-- BinaryHolder.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
 //
 //===----------------------------------------------------------------------===//
 //
 // This program is a utility that aims to be a dropin replacement for
 // Darwin's dsymutil.

 
 Expected<const BinaryHolder::ObjectEntry &>
 BinaryHolder::ArchiveEntry::getObjectEntry(StringRef Filename,
                                            TimestampTy Timestamp,
                                            bool Verbose) {
   StringRef ArchiveFilename;
   StringRef ObjectFilename;
   std::tie(ArchiveFilename, ObjectFilename) = getArchiveAndObjectName(Filename);
-
-  // Try the cache first.
   KeyTy Key = {ObjectFilename, Timestamp};
 
-  {
-    std::lock_guard<std::mutex> Lock(MemberCacheMutex);
-    if (MemberCache.count(Key))
-      return MemberCache[Key];
-  }
+  // Try the cache first.
+    return *MemberCache[Key].get();
 
   // Create a new ObjectEntry, but don't add it to the cache yet. Loading of
   // the archive members might fail and we don't want to lock the whole archive
   // during this operation.
-  ObjectEntry OE;
+  auto OE = std::make_unique<ObjectEntry>();
 
   for (const auto &Archive : Archives) {
     Error Err = Error::success();
     for (auto Child : Archive->children(Err)) {
       if (auto NameOrErr = Child.getName()) {
         if (*NameOrErr == ObjectFilename) {
           auto ModTimeOrErr = Child.getLastModified();
           if (!ModTimeOrErr)
for (auto Child : Archive->children(Err)) {
           if (!ErrOrMem)
             return ErrOrMem.takeError();
 
           auto ErrOrObjectFile =
               object::ObjectFile::createObjectFile(*ErrOrMem);
           if (!ErrOrObjectFile)
             return ErrOrObjectFile.takeError();
 
-          OE.Objects.push_back(std::move(*ErrOrObjectFile));
+          OE->Objects.push_back(std::move(*ErrOrObjectFile));
         }
       }
     }
     if (Err)
       return std::move(Err);
   }
 
-  if (OE.Objects.empty())
+  if (OE->Objects.empty())
     return errorCodeToError(errc::no_such_file_or_directory);
 
-  std::lock_guard<std::mutex> Lock(MemberCacheMutex);
-  MemberCache.try_emplace(Key, std::move(OE));
-  return MemberCache[Key];
+  MemberCache[Key] = std::move(OE);
+  return *MemberCache[Key];
 }
 
 Expected<const BinaryHolder::ObjectEntry &>
 BinaryHolder::getObjectEntry(StringRef Filename, TimestampTy Timestamp) {
   if (Verbose)
     WithColor::note() << "trying to open '" << Filename << "'\n";
 
   // If this is an archive, we might have either the object or the archive
   // cached. In this case we can load it without accessing the file system.
   if (isArchive(Filename)) {
     StringRef ArchiveFilename = getArchiveAndObjectName(Filename).first;
     std::lock_guard<std::mutex> Lock(ArchiveCacheMutex);
     if (ArchiveCache.count(ArchiveFilename)) {
-      return ArchiveCache[ArchiveFilename].getObjectEntry(Filename, Timestamp,
+      return ArchiveCache[ArchiveFilename]->getObjectEntry(Filename, Timestamp,
                                                            Verbose);
     } else {
-      ArchiveEntry &AE = ArchiveCache[ArchiveFilename];
-      auto Err = AE.load(VFS, Filename, Timestamp, Verbose);
+      auto AE = std::make_unique<ArchiveEntry>();
+      auto Err = AE->load(VFS, Filename, Timestamp, Verbose);
       if (Err) {
-        ArchiveCache.erase(ArchiveFilename);
         // Don't return the error here: maybe the file wasn't an archive.
         llvm::consumeError(std::move(Err));
       } else {
-        return ArchiveCache[ArchiveFilename].getObjectEntry(Filename, Timestamp,
-                                                            Verbose);
+        ArchiveCache[ArchiveFilename] = std::move(AE);
+        return ArchiveCache[ArchiveFilename]->getObjectEntry(
+            Filename, Timestamp, Verbose);
       }
     }
   }
 
   // If this is an object, we might have it cached. If not we'll have to load
   // it from the file system and cache it now.
   std::lock_guard<std::mutex> Lock(ObjectCacheMutex);
   if (!ObjectCache.count(Filename)) {
-    ObjectEntry &OE = ObjectCache[Filename];
-    auto Err = OE.load(VFS, Filename, Timestamp, Verbose);
-    if (Err) {
-      ObjectCache.erase(Filename);
+    auto OE = std::make_unique<ObjectEntry>();
+    auto Err = OE->load(VFS, Filename, Timestamp, Verbose);
+    if (Err)
       return std::move(Err);
-    }
+    ObjectCache[Filename] = std::move(OE);
   }
 
-  return ObjectCache[Filename];
+  return *ObjectCache[Filename];
 }
 
 void BinaryHolder::clear() {
   std::lock_guard<std::mutex> ArchiveLock(ArchiveCacheMutex);
   std::lock_guard<std::mutex> ObjectLock(ObjectCacheMutex);
   ArchiveCache.clear();
   ObjectCache.clear();
 }
 
 } // namespace dsymutil
 } // namespace llvm