From b89da20e440e08989fa0f613f84f593c3cd62580 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 9 Nov 2026 15:20:17 -0600 Subject: [PATCH 1/2] Unbind a ClassLoader from its library when it is destroyed A ClassLoader destroyed while instances it created were still alive (in either mode), or after loadLibrary() was called more than once, never unloaded its library and stayed registered as an owner of the library's classes. The library then stayed loaded for the rest of the process, and a later ClassLoader allocated at the same address, even one for a different library, appeared to own those classes. Now the destructor always unbinds the loader from its library. If managed instances are still alive, it first opens another handle to the library for them, and the last of them closes it. Signed-off-by: Michael Carroll --- include/class_loader/class_loader.hpp | 11 +++- src/class_loader.cpp | 42 +++++++++++---- test/utest.cpp | 78 +++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 10 deletions(-) diff --git a/include/class_loader/class_loader.hpp b/include/class_loader/class_loader.hpp index 27da8cf..d9ada57 100644 --- a/include/class_loader/class_loader.hpp +++ b/include/class_loader/class_loader.hpp @@ -275,7 +275,12 @@ class ClassLoader if (context->loader) { context->loader->onPluginDeletion(p); } else { - delete p; // The library is still loaded: it is not unloaded while instances exist. + // The ClassLoader is gone, but its handle in context->library kept the library + // loaded. The last instance closes it. + delete p; + if (context->orphaned_instances > 0 && --context->orphaned_instances == 0) { + context->library.reset(); + } } }; } @@ -400,6 +405,10 @@ class ClassLoader { std::recursive_mutex mutex; ClassLoader * loader = nullptr; // Cleared when the ClassLoader is destroyed. + // Set when the ClassLoader is destroyed while managed instances still exist: a handle of + // their own that keeps the library loaded, and how many instances still need it. + std::shared_ptr library; + std::size_t orphaned_instances = 0; }; std::shared_ptr deleter_context_; }; diff --git a/src/class_loader.cpp b/src/class_loader.cpp index 66d98a8..c759f95 100644 --- a/src/class_loader.cpp +++ b/src/class_loader.cpp @@ -75,17 +75,41 @@ ClassLoader::~ClassLoader() "class_loader.ClassLoader: Destroying class loader, " "unloading associated library...\n"); std::lock_guard lock(deleter_context_->mutex); - { - // The deferred on-demand unload in onPluginDeletion() can't happen once this loader is gone. - std::lock_guard ref_count_lock(plugin_ref_count_mutex_); - if (plugin_ref_count_ > 0 && isOnDemandLoadUnloadEnabled()) { - CONSOLE_BRIDGE_logWarn( - "%s", - "class_loader.ClassLoader: SEVERE WARNING!!! " - "Destroying ClassLoader while objects created by it exist in the heap!"); + std::lock_guard ref_count_lock(plugin_ref_count_mutex_); + std::lock_guard load_ref_lock(load_ref_count_mutex_); + + if (getLibraryPath().empty() || load_ref_count_ <= 0) { + deleter_context_->loader = nullptr; + return; + } + + if (plugin_ref_count_ > 0) { + // Managed instances outlive this loader. Open another handle to the library for them, so it + // stays loaded until the last of them is deleted (see makeDeleter()), even once nothing else + // uses it. + try { + deleter_context_->library = std::make_shared(getLibraryPath()); + deleter_context_->orphaned_instances = static_cast(plugin_ref_count_); + } catch (const std::exception & e) { + CONSOLE_BRIDGE_logError( + "class_loader.ClassLoader: Destroying ClassLoader for %s while objects created by it " + "exist, and could not keep the library loaded for them (%s). The library will stay " + "loaded.", getLibraryPath().c_str(), e.what()); + deleter_context_->loader = nullptr; + return; } } - unloadLibrary(); // TODO(mikaelarguedas): while(unloadLibrary() > 0){} ?? + + // Unbind from the library however many times loadLibrary() was called. Leaving this loader + // registered as an owner of the library's classes would leave a dangling pointer behind, and a + // later ClassLoader at the same address would appear to own them. + try { + class_loader::impl::unloadLibrary(getLibraryPath(), this); + } catch (const class_loader::LibraryUnloadException & e) { + CONSOLE_BRIDGE_logError( + "class_loader.ClassLoader: Could not unload %s: %s", getLibraryPath().c_str(), e.what()); + } + load_ref_count_ = 0; deleter_context_->loader = nullptr; } diff --git a/test/utest.cpp b/test/utest.cpp index e9c3da1..5937aa4 100644 --- a/test/utest.cpp +++ b/test/utest.cpp @@ -34,6 +34,7 @@ #endif #include +#include #include #include #include @@ -387,8 +388,85 @@ TEST(ClassLoaderBasicTest, isLibraryLoadedScopedToLoader) { ASSERT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); } +#ifndef _WIN32 +// Whether the dynamic linker still has the library mapped, whatever class_loader thinks. +bool isLibraryMapped(const std::string & library_path) +{ + void * handle = dlopen(library_path.c_str(), RTLD_LAZY | RTLD_NOLOAD); + if (handle == nullptr) { + return false; + } + dlclose(handle); + return true; +} + +// Instances that outlive their ClassLoader keep the library loaded, and the last of them unloads +// it, in both on-demand and non-on-demand mode. +TEST(ClassLoaderLifetimeTest, lastOrphanedInstanceUnloadsLibrary) { + for (bool on_demand : {false, true}) { + SCOPED_TRACE(on_demand ? "on-demand" : "non-on-demand"); + ASSERT_FALSE(isLibraryMapped(LIBRARY_2)); + auto loader = std::make_unique(LIBRARY_2, on_demand); + std::shared_ptr shared = loader->createInstance("Robot"); + class_loader::ClassLoader::UniquePtr unique = loader->createUniqueInstance("Robot"); + loader.reset(); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_2)); + EXPECT_TRUE(isLibraryMapped(LIBRARY_2)); + + shared.reset(); + EXPECT_EQ("Beep boop", unique->saySomething()); + EXPECT_TRUE(isLibraryMapped(LIBRARY_2)); + unique.reset(); + EXPECT_FALSE(isLibraryMapped(LIBRARY_2)); + } +} +#endif + +// The library can be loaded again while instances that outlived their ClassLoader still exist. +TEST(ClassLoaderLifetimeTest, reloadWhileOrphanedInstancesExist) { + auto loader = std::make_unique(LIBRARY_2, false); + std::shared_ptr orphan = loader->createInstance("Robot"); + loader.reset(); + + class_loader::ClassLoader loader2(LIBRARY_2, false); + EXPECT_EQ("Beep boop", loader2.createInstance("Robot")->saySomething()); + orphan.reset(); + EXPECT_TRUE(loader2.isLibraryLoaded()); + EXPECT_EQ("Beep boop", loader2.createUniqueInstance("Robot")->saySomething()); +} + +// A destroyed ClassLoader must not stay registered as an owner of its library's classes. If it +// did, a new ClassLoader at the same address could see and create them without loading anything. +TEST(ClassLoaderLifetimeTest, destroyedLoaderOwnsNothing) { + enum class Case { kLiveInstance, kLiveInstanceOnDemand, kLoadedTwice }; + for (Case c : {Case::kLiveInstance, Case::kLiveInstanceOnDemand, Case::kLoadedTwice}) { + SCOPED_TRACE(static_cast(c)); + alignas(class_loader::ClassLoader) unsigned char storage[sizeof(class_loader::ClassLoader)]; + std::shared_ptr dog; + { + auto * loader = new (storage) class_loader::ClassLoader( + LIBRARY_1, c == Case::kLiveInstanceOnDemand); + if (c == Case::kLoadedTwice) { + loader->loadLibrary(); + } else { + dog = loader->createInstance("Dog"); + } + loader->~ClassLoader(); + } + { + auto * other = new (storage) class_loader::ClassLoader(LIBRARY_2, true); + EXPECT_FALSE(other->isClassAvailable("Dog")); + other->~ClassLoader(); + } + dog.reset(); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + } +} + #ifndef _WIN32 // Not run on Windows because this tests dlopen-specific behavior +// It also opens a plugin library outside class_loader, after which class_loader never unloads any +// library in this process, so tests that check unloading must come before it. // This is a different class name so that static variables in ClassLoader are isolated TEST(ClassLoaderGraveyardTest, loadUnloadLoadFromGraveyard) { From 4b09b63aee4f4d9ec1a957e7f17bfdb402ab79f8 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Mon, 9 Nov 2026 15:20:17 -0600 Subject: [PATCH 2/2] Fix data races on metaobject ownership and add stress tests isLibraryLoaded() and the allMetaObjectsFor*() helpers filtered metaobjects without holding the factory map lock, while other threads changed their owners or destroyed them under it, and createInstance() checked ownership after releasing it. ThreadSanitizer reported these races, and loading and unloading the same library on several threads could fail to find a class that was loaded. Check and filter under the lock, keeping the existing lock order. Add stress tests for loaders and MultiLibraryClassLoaders sharing libraries across threads, and for instances released on other threads while their loader is destroyed. Signed-off-by: Michael Carroll --- include/class_loader/class_loader_core.hpp | 8 ++- src/class_loader_core.cpp | 9 +++ test/utest.cpp | 81 ++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/include/class_loader/class_loader_core.hpp b/include/class_loader/class_loader_core.hpp index 418418b..2548804 100644 --- a/include/class_loader/class_loader_core.hpp +++ b/include/class_loader/class_loader_core.hpp @@ -355,12 +355,16 @@ Base * createInstance(const std::string & derived_class_name, ClassLoader * load CONSOLE_BRIDGE_logError( "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str()); } + // Check ownership under the lock (other threads change it), but construct outside it. + const bool owned_by_loader = factory != nullptr && factory->isOwnedBy(loader); + const bool owned_by_nobody = factory != nullptr && !owned_by_loader && + factory->isOwnedBy(nullptr); getPluginBaseToFactoryMapMapMutex().unlock(); Base * obj = nullptr; - if (factory != nullptr && factory->isOwnedBy(loader)) { + if (owned_by_loader) { obj = factory->create(std::forward(args)...); - } else if (factory && factory->isOwnedBy(nullptr)) { + } else if (owned_by_nobody) { CONSOLE_BRIDGE_logDebug( "%s", "class_loader.impl: ALERT!!! " diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp index 6c08c37..0a64eb1 100644 --- a/src/class_loader_core.cpp +++ b/src/class_loader_core.cpp @@ -195,18 +195,24 @@ filterAllMetaObjectsAssociatedWithLibrary( MetaObjectVector allMetaObjectsForClassLoader(const ClassLoader * owner) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsOwnedBy(allMetaObjects(), owner); } MetaObjectVector allMetaObjectsForLibrary(const std::string & library_path) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsAssociatedWithLibrary(allMetaObjects(), library_path); } MetaObjectVector allMetaObjectsForLibraryOwnedBy(const std::string & library_path, const ClassLoader * owner) { + // Filter under the lock too: other threads change owners and destroy metaobjects under it. + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); return filterAllMetaObjectsOwnedBy(allMetaObjectsForLibrary(library_path), owner); } @@ -302,6 +308,9 @@ bool isLibraryLoadedByAnybody(const std::string & library_path) bool isLibraryLoaded(const std::string & library_path, const ClassLoader * loader) { + // Answer from one consistent state, in the same lock order as unloadLibrary(). + std::lock_guard llv_lock(getLoadedLibraryVectorMutex()); + std::lock_guard b2fmm_lock(getPluginBaseToFactoryMapMapMutex()); bool is_lib_loaded_by_anyone = isLibraryLoadedByAnybody(library_path); size_t num_meta_objs_for_lib = allMetaObjectsForLibrary(library_path).size(); size_t num_meta_objs_for_lib_bound_to_loader = diff --git a/test/utest.cpp b/test/utest.cpp index 5937aa4..3c9a608 100644 --- a/test/utest.cpp +++ b/test/utest.cpp @@ -33,6 +33,8 @@ #include #endif +#include +#include #include #include #include @@ -463,6 +465,85 @@ TEST(ClassLoaderLifetimeTest, destroyedLoaderOwnsNothing) { } } +// Stress tests for state shared between loaders. Mostly useful under ThreadSanitizer. +constexpr int STRESS_ITERATIONS = 20; +constexpr size_t STRESS_THREADS = 8; + +TEST(ClassLoaderStressTest, loadersForSameLibraryOnManyThreads) { + std::vector threads; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + threads.emplace_back( + [t]() { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + class_loader::ClassLoader loader(LIBRARY_1, (t + i) % 2 == 0); + EXPECT_EQ("Bark", loader.createInstance("Dog")->saySomething()); + EXPECT_EQ("Meow", loader.createUniqueInstance("Cat")->saySomething()); + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); +} + +TEST(ClassLoaderStressTest, multiLibraryClassLoadersOnManyThreads) { + std::vector threads; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + threads.emplace_back( + [t]() { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + class_loader::MultiLibraryClassLoader loader((t + i) % 2 == 0); + loader.loadLibrary(LIBRARY_1); + loader.loadLibrary(LIBRARY_2); + if ((t + i) % 2 != 0) { + // An on-demand loader only lists the classes of libraries it has actually loaded. + const auto classes = loader.getAvailableClasses(); + EXPECT_NE(classes.end(), std::ranges::find(classes, "Dog")); + EXPECT_NE(classes.end(), std::ranges::find(classes, "Robot")); + } + EXPECT_EQ("Bark", loader.createInstance("Dog")->saySomething()); + EXPECT_EQ("Beep boop", loader.createUniqueInstance("Robot")->saySomething()); + loader.unloadLibrary(LIBRARY_1); + } + }); + } + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_2)); +} + +// Instances deleted on other threads while their ClassLoader is being destroyed. +TEST(ClassLoaderStressTest, instancesReleasedWhileLoaderDestroyed) { + for (int i = 0; i < STRESS_ITERATIONS; ++i) { + auto loader = std::make_unique(LIBRARY_1, i % 2 == 0); + std::vector> instances; + for (size_t t = 0; t < STRESS_THREADS; ++t) { + instances.push_back(loader->createInstance("Dog")); + } + std::atomic start{false}; + std::vector threads; + for (auto & instance : instances) { + threads.emplace_back( + [&start, instance = std::move(instance)]() mutable { + while (!start.load()) { + std::this_thread::yield(); + } + EXPECT_EQ("Bark", instance->saySomething()); + instance.reset(); + }); + } + start = true; + loader.reset(); + for (auto & thread : threads) { + thread.join(); + } + EXPECT_FALSE(class_loader::impl::isLibraryLoadedByAnybody(LIBRARY_1)); + } +} + #ifndef _WIN32 // Not run on Windows because this tests dlopen-specific behavior // It also opens a plugin library outside class_loader, after which class_loader never unloads any