Fix data race on AbstractMetaObjectBase's owning-loader list

include/class_loader/class_loader_core.hpp

@@ -346,21 +346,31 @@ requires InterfaceConstructible<Base, Args...>
 Base * createInstance(const std::string & derived_class_name, ClassLoader * loader, Args &&... args)
 {
   AbstractMetaObject<Base> * factory = nullptr;
+  bool owned_by_loader = false;
+  bool owned_by_none = false;
 
-  getPluginBaseToFactoryMapMapMutex().lock();
-  FactoryMap & factoryMap = getFactoryMapForBaseClass<Base>();
-  if (auto it = factoryMap.find(derived_class_name); it != factoryMap.end()) {
-    factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(it->second);
-  } else {
-    CONSOLE_BRIDGE_logError(
-      "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str());
+  {
+    std::lock_guard<std::recursive_mutex> lock(getPluginBaseToFactoryMapMapMutex());
+    FactoryMap & factoryMap = getFactoryMapForBaseClass<Base>();
+    if (auto it = factoryMap.find(derived_class_name); it != factoryMap.end()) {
+      factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(it->second);
+    } else {
+      CONSOLE_BRIDGE_logError(
+        "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str());
+    }
+    if (factory != nullptr) {
+      // isOwnedBy() must be read while still holding this mutex: another thread's
+      // addOwningClassLoader()/removeOwningClassLoader() (e.g. from loadLibrary()'s
+      // already-loaded branch) is only serialized against this same mutex.
+      owned_by_loader = factory->isOwnedBy(loader);
+      owned_by_none = factory->isOwnedBy(nullptr);
+    }
   }
-  getPluginBaseToFactoryMapMapMutex().unlock();
 
   Base * obj = nullptr;
-  if (factory != nullptr && factory->isOwnedBy(loader)) {
+  if (factory != nullptr && owned_by_loader) {
     obj = factory->create(std::forward<Args>(args)...);
-  } else if (factory && factory->isOwnedBy(nullptr)) {
+  } else if (factory && owned_by_none) {
     CONSOLE_BRIDGE_logDebug(
       "%s",
       "class_loader.impl: ALERT!!! "

src/meta_object.cpp

@@ -29,6 +29,7 @@
 
 #include <algorithm>
 #include <memory>
+#include <mutex>
 #include <string>
 #include <vector>
 
@@ -45,6 +46,10 @@ using ClassLoaderVector = std::vector<class_loader::ClassLoader *>;
 class AbstractMetaObjectBaseImpl
 {
 public:
+  // Guards associated_class_loaders_: class_loader_core.hpp's createInstance() reads
+  // isOwnedBy() after releasing getPluginBaseToFactoryMapMapMutex(), so this vector needs
+  // its own lock independent of that outer mutex.
+  mutable std::recursive_mutex associated_class_loaders_mutex_;
   ClassLoaderVector associated_class_loaders_;
   std::string associated_library_path_;
   std::string base_class_name_;
@@ -102,6 +107,7 @@ void AbstractMetaObjectBase::setAssociatedLibraryPath(const std::string & librar
 
 void AbstractMetaObjectBase::addOwningClassLoader(ClassLoader * loader)
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   ClassLoaderVector & v = impl_->associated_class_loaders_;
   if (std::ranges::find(v, loader) == v.end()) {
     v.push_back(loader);
@@ -110,6 +116,7 @@ void AbstractMetaObjectBase::addOwningClassLoader(ClassLoader * loader)
 
 void AbstractMetaObjectBase::removeOwningClassLoader(const ClassLoader * loader)
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   ClassLoaderVector & v = impl_->associated_class_loaders_;
   if (auto itr = std::ranges::find(v, loader); itr != v.end()) {
     v.erase(itr);
@@ -118,22 +125,26 @@ void AbstractMetaObjectBase::removeOwningClassLoader(const ClassLoader * loader)
 
 bool AbstractMetaObjectBase::isOwnedBy(const ClassLoader * loader) const
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   const ClassLoaderVector & v = impl_->associated_class_loaders_;
   return std::ranges::find(v, loader) != v.end();
 }
 
 bool AbstractMetaObjectBase::isOwnedByAnybody() const
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   return impl_->associated_class_loaders_.size() > 0;
 }
 
 size_t AbstractMetaObjectBase::getAssociatedClassLoadersCount() const
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   return impl_->associated_class_loaders_.size();
 }
 
 ClassLoader * AbstractMetaObjectBase::getAssociatedClassLoader(size_t index) const
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);
   return impl_->associated_class_loaders_[index];
 }
 

test/utest.cpp

@@ -27,6 +27,7 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <atomic>
 #include <chrono>
 #include <cstddef>
 
@@ -307,6 +308,50 @@ TEST(ClassLoaderTest, threadSafety) {
   }
 }
 
+// Regression test: unlike threadSafety above (one ClassLoader shared by every thread, so
+// isOwnedBy() is always queried with the loader that already, and permanently, owns the
+// metaobject), this constructs a *separate* ClassLoader per thread for the same library.
+// Each construction/destruction concurrently mutates AbstractMetaObjectBase's
+// associated_class_loaders_ vector (via addOwningClassLoader()/removeOwningClassLoader())
+// while other threads concurrently read it via isOwnedBy() in createInstance() - previously
+// unguarded, since createInstance() released getPluginBaseToFactoryMapMapMutex() before
+// reading isOwnedBy(), and addOwningClassLoader()/removeOwningClassLoader()/isOwnedBy() took
+// no lock of their own. Reported by ThreadSanitizer as a data race on the vector's
+// push_back()/erase() (via std::vector's internal reallocation).
+//
+// `anchor` is kept alive for the whole test so the library's owner count never reaches zero
+// and none of the worker threads ever trigger the actual unload/dlclose path: that path has
+// its own pre-existing, unrelated bug (a library can be dlclose()-d twice when independent
+// ClassLoaders for the same library race to be "last owner out"), which this test isn't
+// targeting and would otherwise crash it intermittently regardless of the fix under test here.
+TEST(ClassLoaderTest, threadSafetyMultipleLoadersPerLibrary) {
+  class_loader::ClassLoader anchor(LIBRARY_1);
+
+  std::vector<std::thread> client_threads;
+  std::atomic<int> failures{0};
+
+  for (size_t c = 0; c < STRESS_TEST_NUM_THREADS; ++c) {
+    client_threads.emplace_back(
+      [&failures]() {
+        try {
+          class_loader::ClassLoader loader(LIBRARY_1);
+          std::vector<std::string> classes = loader.getAvailableClasses<Base>();
+          for (auto & class_name : classes) {
+            loader.createInstance<Base>(class_name)->saySomething();
+          }
+        } catch (...) {
+          ++failures;
+        }
+      });
+  }
+
+  for (auto & client_thread : client_threads) {
+    client_thread.join();
+  }
+
+  EXPECT_EQ(0, failures.load());
+}
+
 TEST(ClassLoaderTest, loadRefCountingNonLazy) {
   try {
     class_loader::ClassLoader loader1(LIBRARY_1, false);