Don't use a destroyed ClassLoader when deleting its instances

include/class_loader/class_loader.hpp

@@ -132,7 +132,7 @@ class ClassLoader
   {
     return std::shared_ptr<Base>(
       createRawInstance<Base>(derived_class_name, true, std::forward<Args>(args)...),
-      [this](Base * p) {onPluginDeletion<Base>(p);}
+      makeDeleter<Base>()
     );
   }
 
@@ -159,7 +159,7 @@ class ClassLoader
     Base * raw = createRawInstance<Base>(derived_class_name, true, std::forward<Args>(args)...);
     return std::unique_ptr<Base, DeleterType<Base>>(
       raw,
-      [this](Base * p) {onPluginDeletion<Base>(p);}
+      makeDeleter<Base>()
     );
   }
 
@@ -266,6 +266,20 @@ class ClassLoader
   int unloadLibrary();
 
 private:
+  /// Deleter for managed instances that stays valid if the instance outlives this ClassLoader.
+  template<class Base>
+  DeleterType<Base> makeDeleter()
+  {
+    return [context = deleter_context_](Base * p) {
+             std::lock_guard<std::recursive_mutex> lock(context->mutex);
+             if (context->loader) {
+               context->loader->onPluginDeletion<Base>(p);
+             } else {
+               delete p;  // The library is still loaded: it is not unloaded while instances exist.
+             }
+           };
+  }
+
   /**
    * @brief Callback method when a plugin created by this class loader is destroyed
    *
@@ -381,6 +395,13 @@ class ClassLoader
   int plugin_ref_count_;
   std::recursive_mutex plugin_ref_count_mutex_;
   static std::atomic<bool> has_unmanaged_instance_been_created_;
+
+  struct DeleterContext
+  {
+    std::recursive_mutex mutex;
+    ClassLoader * loader = nullptr;  // Cleared when the ClassLoader is destroyed.
+  };
+  std::shared_ptr<DeleterContext> deleter_context_;
 };
 
 }  // namespace class_loader

src/class_loader.cpp

@@ -55,8 +55,10 @@ ClassLoader::ClassLoader(const std::string & library_path, bool ondemand_load_un
 : ondemand_load_unload_(ondemand_load_unload),
   library_path_(library_path),
   load_ref_count_(0),
-  plugin_ref_count_(0)
+  plugin_ref_count_(0),
+  deleter_context_(std::make_shared<DeleterContext>())
 {
+  deleter_context_->loader = this;
   CONSOLE_BRIDGE_logDebug(
     "class_loader.ClassLoader: "
     "Constructing new ClassLoader (%p) bound to library %s.",
@@ -72,9 +74,10 @@ ClassLoader::~ClassLoader()
     "%s",
     "class_loader.ClassLoader: Destroying class loader, "
     "unloading associated library...\n");
+  std::lock_guard<std::recursive_mutex> lock(deleter_context_->mutex);
   {
     // The deferred on-demand unload in onPluginDeletion() can't happen once this loader is gone.
-    std::lock_guard<std::recursive_mutex> lock(plugin_ref_count_mutex_);
+    std::lock_guard<std::recursive_mutex> ref_count_lock(plugin_ref_count_mutex_);
     if (plugin_ref_count_ > 0 && isOnDemandLoadUnloadEnabled()) {
       CONSOLE_BRIDGE_logWarn(
         "%s",
@@ -83,6 +86,7 @@ ClassLoader::~ClassLoader()
     }
   }
   unloadLibrary();  // TODO(mikaelarguedas): while(unloadLibrary() > 0){} ??
+  deleter_context_->loader = nullptr;
 }
 
 const std::string & ClassLoader::getLibraryPath() const

test/utest.cpp

@@ -521,6 +521,19 @@ TEST(ClassLoaderGraveyardTest, loadUnloadLoadFromGraveyard) {
 
 #endif  // ifndef _WIN32
 
+// Leaves LIBRARY_2 loaded, so keep this suite last.
+TEST(ClassLoaderLifetimeTest, instancesOutliveLoader) {
+  auto loader = std::make_unique<class_loader::ClassLoader>(LIBRARY_2, false);
+  std::shared_ptr<Base> shared = loader->createInstance<Base>("Robot");
+  class_loader::ClassLoader::UniquePtr<Base> unique = loader->createUniqueInstance<Base>("Robot");
+  loader.reset();
+  EXPECT_EQ("Beep boop", shared->saySomething());
+  EXPECT_EQ("Beep boop", unique->saySomething());
+  // Must not use the destroyed loader (heap-use-after-free under AddressSanitizer).
+  shared.reset();
+  unique.reset();
+}
+
 // Run all the tests that were declared with TEST()
 int main(int argc, char ** argv)
 {