Fix lifetime errors and memory leaks

.github/workflows/ci.yaml

@@ -0,0 +1,30 @@
+# This config uses industrial_ci (https://github.com/ros-industrial/industrial_ci.git).
+# For troubleshooting, see readme (https://github.com/ros-industrial/industrial_ci/blob/master/README.rst)
+
+name: CI
+
+on:
+  workflow_dispatch:
+  pull_request:
+  push:
+    branches:
+      - ros2
+
+jobs:
+  ci:
+    strategy:
+      fail-fast: false
+      matrix:
+        env:
+          - TARGET_CMAKE_ARGS: -DENABLE_SANITIZER_ADDRESS=ON -DCMAKE_BUILD_TYPE=Debug
+            NAME: Address Sanitizer
+          - TARGET_CMAKE_ARGS: -DENABLE_SANITIZER_THREAD=ON -DCMAKE_BUILD_TYPE=Debug
+            NAME: Thread Sanitizer
+    env:
+      ROS_DISTRO: rolling
+    runs-on: ubuntu-latest
+    name: ${{ matrix.env.NAME }}
+    steps:
+      - uses: actions/checkout@v2
+      - uses: 'ros-industrial/industrial_ci@master'
+        env: ${{matrix.env}}

CMakeLists.txt

@@ -4,7 +4,7 @@ project(class_loader)
 
 # Default to C++14
 if(NOT CMAKE_CXX_STANDARD)
-  set(CMAKE_CXX_STANDARD 14)
+  set(CMAKE_CXX_STANDARD 17)
 endif()
 if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
   add_compile_options(-Wall -Wextra -Wpedantic)

cmake/sanitizers.cmake

@@ -0,0 +1,68 @@
+function(enable_sanitizers project_name)
+
+  if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES ".*Clang")
+    option(ENABLE_COVERAGE "Enable coverage reporting for gcc/clang" OFF)
+
+    if(ENABLE_COVERAGE)
+      target_compile_options(${project_name} INTERFACE --coverage -O0 -g -fno-omit-frame-pointer)
+      target_link_libraries(${project_name} INTERFACE --coverage)
+    endif()
+
+    set(SANITIZERS "")
+
+    option(ENABLE_SANITIZER_ADDRESS "Enable address sanitizer" OFF)
+    if(ENABLE_SANITIZER_ADDRESS)
+      list(APPEND SANITIZERS "address")
+    endif()
+
+    option(ENABLE_SANITIZER_LEAK "Enable leak sanitizer" OFF)
+    if(ENABLE_SANITIZER_LEAK)
+      list(APPEND SANITIZERS "leak")
+    endif()
+
+    option(ENABLE_SANITIZER_UNDEFINED_BEHAVIOR "Enable undefined behavior sanitizer" OFF)
+    if(ENABLE_SANITIZER_UNDEFINED_BEHAVIOR)
+      list(APPEND SANITIZERS "undefined")
+    endif()
+
+    option(ENABLE_SANITIZER_THREAD "Enable thread sanitizer" OFF)
+    if(ENABLE_SANITIZER_THREAD)
+      if("address" IN_LIST SANITIZERS OR "leak" IN_LIST SANITIZERS)
+        message(WARNING "Thread sanitizer does not work with Address and Leak sanitizer enabled")
+      else()
+        list(APPEND SANITIZERS "thread")
+      endif()
+    endif()
+
+    option(ENABLE_SANITIZER_MEMORY "Enable memory sanitizer" OFF)
+    if(ENABLE_SANITIZER_MEMORY AND CMAKE_CXX_COMPILER_ID MATCHES ".*Clang")
+      message(WARNING "Memory sanitizer requires all the code (including libc++) \
+        to be MSan-instrumented otherwise it reports false positives")
+      if("address" IN_LIST SANITIZERS
+          OR "thread" IN_LIST SANITIZERS
+          OR "leak" IN_LIST SANITIZERS)
+        message(WARNING "Memory sanitizer does not work with Address, Thread and Leak sanitizer enabled")
+      else()
+        list(APPEND SANITIZERS "memory")
+      endif()
+    endif()
+
+    list(
+      JOIN
+      SANITIZERS
+      ","
+      LIST_OF_SANITIZERS)
+
+  endif()
+
+  if(LIST_OF_SANITIZERS)
+    if(NOT
+        "${LIST_OF_SANITIZERS}"
+        STREQUAL
+        "")
+      target_compile_options(${project_name} INTERFACE -fsanitize=${LIST_OF_SANITIZERS})
+      target_link_options(${project_name} INTERFACE -fsanitize=${LIST_OF_SANITIZERS})
+    endif()
+  endif()
+
+endfunction()

include/class_loader/class_loader.hpp

@@ -76,7 +76,7 @@ std::string systemLibraryFormat(const std::string & library_name);
  * definitions from which objects can be created/destroyed during runtime (i.e. class_loader).
  * Libraries loaded by a ClassLoader are only accessible within scope of that ClassLoader object.
  */
-class ClassLoader
+class ClassLoader : public std::enable_shared_from_this<ClassLoader>
 {
 public:
   template<typename Base>
@@ -86,14 +86,17 @@ class ClassLoader
   using UniquePtr = std::unique_ptr<Base, DeleterType<Base>>;
 
   /**
-   * @brief Constructor for ClassLoader
+   * @brief This is required (as apposed to the old constructor) to enforce that the lifetime of the ClassLoader
+   * can be preserved by objects that this class creates.
    *
    * @param library_path - The path of the runtime library to load
    * @param ondemand_load_unload - Indicates if on-demand (lazy) unloading/loading of libraries
    * occurs as plugins are created/destroyed.
    */
   CLASS_LOADER_PUBLIC
-  explicit ClassLoader(const std::string & library_path, bool ondemand_load_unload = false);
+  [[nodiscard]] static std::shared_ptr<ClassLoader> Make(
+    const std::string & library_path,
+    bool ondemand_load_unload = false);
 
   /**
    * @brief Destructor for ClassLoader. All libraries opened by this ClassLoader are unloaded automatically.
@@ -109,7 +112,7 @@ class ClassLoader
   template<class Base>
   std::vector<std::string> getAvailableClasses() const
   {
-    return class_loader::impl::getAvailableClasses<Base>(this);
+    return class_loader::impl::getAvailableClasses<Base>(shared_from_this());
   }
 
   /**
@@ -126,7 +129,9 @@ class ClassLoader
   {
     return std::shared_ptr<Base>(
       createRawInstance<Base>(derived_class_name, true),
-      std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
+      [class_loader = shared_from_this()](Base * obj) {
+        return class_loader->onPluginDeletion<Base>(obj);
+      }
     );
   }
 
@@ -149,7 +154,9 @@ class ClassLoader
     Base * raw = createRawInstance<Base>(derived_class_name, true);
     return std::unique_ptr<Base, DeleterType<Base>>(
       raw,
-      std::bind(&ClassLoader::onPluginDeletion<Base>, this, std::placeholders::_1)
+      [class_loader = shared_from_this()](Base * obj) {
+        return class_loader->onPluginDeletion<Base>(obj);
+      }
     );
   }
 
@@ -253,6 +260,16 @@ class ClassLoader
   int unloadLibrary();
 
 private:
+  /**
+   * @brief Private Constructor for ClassLoader
+   *
+   * @param library_path - The path of the runtime library to load
+   * @param ondemand_load_unload - Indicates if on-demand (lazy) unloading/loading of libraries
+   * occurs as plugins are created/destroyed.
+   */
+  CLASS_LOADER_PUBLIC
+  explicit ClassLoader(const std::string & library_path, bool ondemand_load_unload = false);
+
   /**
    * @brief Callback method when a plugin created by this class loader is destroyed
    *
@@ -324,7 +341,7 @@ class ClassLoader
       loadLibrary();
     }
 
-    Base * obj = class_loader::impl::createInstance<Base>(derived_class_name, this);
+    Base * obj = class_loader::impl::createInstance<Base>(derived_class_name, shared_from_this());
     assert(obj != NULL);  // Unreachable assertion if createInstance() throws on failure.
 
     if (managed) {

include/class_loader/class_loader_core.hpp

@@ -76,11 +76,14 @@ namespace impl
 typedef std::string LibraryPath;
 typedef std::string ClassName;
 typedef std::string BaseClassName;
-typedef std::map<ClassName, impl::AbstractMetaObjectBase *> FactoryMap;
+typedef std::map<ClassName, std::shared_ptr<AbstractMetaObjectBase>> FactoryMap;
 typedef std::map<BaseClassName, FactoryMap> BaseToFactoryMapMap;
 typedef std::pair<LibraryPath, std::shared_ptr<rcpputils::SharedLibrary>> LibraryPair;
 typedef std::vector<LibraryPair> LibraryVector;
-typedef std::vector<AbstractMetaObjectBase *> MetaObjectVector;
+typedef std::vector<std::shared_ptr<AbstractMetaObjectBase>> MetaObjectVector;
+
+CLASS_LOADER_PUBLIC
+MetaObjectVector & getMetaObjectGraveyard();
 
 CLASS_LOADER_PUBLIC
 void printDebugInfoToScreen();
@@ -139,8 +142,15 @@ ClassLoader * getCurrentlyActiveClassLoader();
  * @param loader - pointer to the currently active ClassLoader.
  */
 CLASS_LOADER_PUBLIC
-void setCurrentlyActiveClassLoader(ClassLoader * loader);
+void setCurrentlyActiveClassLoader(std::shared_ptr<ClassLoader> loader);
 
+/**
+ * @brief Inserts meta object into the graveyard to preserve the lifetime.
+ *
+ * @param meta_obj - pointer to the meta object.
+ */
+CLASS_LOADER_PUBLIC
+void insertMetaObjectIntoGraveyard(std::shared_ptr<AbstractMetaObjectBase> meta_obj);
 
 /**
  * @brief This function extracts a reference to the FactoryMap for appropriate base class out of
@@ -240,8 +250,8 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
   }
 
   // Create factory
-  impl::AbstractMetaObject<Base> * new_factory =
-    new impl::MetaObject<Derived, Base>(class_name, base_class_name);
+  auto new_factory =
+    std::make_shared<impl::MetaObject<Derived, Base>>(class_name, base_class_name);
   new_factory->addOwningClassLoader(getCurrentlyActiveClassLoader());
   new_factory->setAssociatedLibraryPath(getCurrentlyLoadingLibraryName());
 
@@ -260,13 +270,17 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
       "and use either class_loader::ClassLoader/MultiLibraryClassLoader to open.",
       class_name.c_str());
   }
+
+  // We insert every factory into the graveyard to preserve the lifetime of all meta objects
+  // until the process exits.
+  insertMetaObjectIntoGraveyard(new_factory);
   factoryMap[class_name] = new_factory;
   getPluginBaseToFactoryMapMapMutex().unlock();
 
   CONSOLE_BRIDGE_logDebug(
     "class_loader.impl: "
     "Registration of %s complete (Metaobject Address = %p)",
-    class_name.c_str(), reinterpret_cast<void *>(new_factory));
+    class_name.c_str(), reinterpret_cast<void *>(new_factory.get()));
 }
 
 /**
@@ -278,22 +292,22 @@ void registerPlugin(const std::string & class_name, const std::string & base_cla
  * @return A pointer to newly created plugin, note caller is responsible for object destruction
  */
 template<typename Base>
-Base * createInstance(const std::string & derived_class_name, ClassLoader * loader)
+Base * createInstance(const std::string & derived_class_name, std::shared_ptr<ClassLoader> loader)
 {
   AbstractMetaObject<Base> * factory = nullptr;
 
   getPluginBaseToFactoryMapMapMutex().lock();
   FactoryMap & factoryMap = getFactoryMapForBaseClass<Base>();
   if (factoryMap.find(derived_class_name) != factoryMap.end()) {
-    factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(factoryMap[derived_class_name]);
+    factory = dynamic_cast<impl::AbstractMetaObject<Base> *>(factoryMap[derived_class_name].get());
   } else {
     CONSOLE_BRIDGE_logError(
       "class_loader.impl: No metaobject exists for class type %s.", derived_class_name.c_str());
   }
   getPluginBaseToFactoryMapMapMutex().unlock();
 
   Base * obj = nullptr;
-  if (factory != nullptr && factory->isOwnedBy(loader)) {
+  if (factory != nullptr && factory->isOwnedBy(loader.get())) {
     obj = factory->create();
   }
 
@@ -333,7 +347,7 @@ Base * createInstance(const std::string & derived_class_name, ClassLoader * load
  * @return A vector of strings where each string is a plugin we can create
  */
 template<typename Base>
-std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
+std::vector<std::string> getAvailableClasses(std::shared_ptr<const ClassLoader> loader)
 {
   std::lock_guard<std::recursive_mutex> lock(getPluginBaseToFactoryMapMapMutex());
 
@@ -342,8 +356,8 @@ std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
   std::vector<std::string> classes_with_no_owner;
 
   for (auto & it : factory_map) {
-    AbstractMetaObjectBase * factory = it.second;
-    if (factory->isOwnedBy(loader)) {
+    auto factory = it.second;
+    if (factory->isOwnedBy(loader.get())) {
       classes.push_back(it.first);
     } else if (factory->isOwnedBy(nullptr)) {
       classes_with_no_owner.push_back(it.first);
@@ -364,7 +378,8 @@ std::vector<std::string> getAvailableClasses(const ClassLoader * loader)
  *   within a ClassLoader's visible scope
  */
 CLASS_LOADER_PUBLIC
-std::vector<std::string> getAllLibrariesUsedByClassLoader(const ClassLoader * loader);
+std::vector<std::string> getAllLibrariesUsedByClassLoader(
+  std::shared_ptr<const ClassLoader> loader);
 
 /**
  * @brief Indicates if passed library loaded within scope of a ClassLoader.
@@ -375,7 +390,7 @@ std::vector<std::string> getAllLibrariesUsedByClassLoader(const ClassLoader * lo
  * @return true if the library is loaded within loader's scope, else false
  */
 CLASS_LOADER_PUBLIC
-bool isLibraryLoaded(const std::string & library_path, const ClassLoader * loader);
+bool isLibraryLoaded(const std::string & library_path, std::shared_ptr<const ClassLoader> loader);
 
 /**
  * @brief Indicates if passed library has been loaded by ANY ClassLoader

include/class_loader/meta_object.hpp

@@ -64,12 +64,11 @@ class CLASS_LOADER_PUBLIC AbstractMetaObjectBase
     const std::string & class_name,
     const std::string & base_class_name,
     const std::string & typeid_base_class_name = "UNSET");
+
   /**
-   * @brief Destructor for the class. THIS MUST NOT BE VIRTUAL AND OVERRIDDEN BY
-   * TEMPLATE SUBCLASSES, OTHERWISE THEY WILL PULL IN A REDUNDANT METAOBJECT
-   * DESTRUCTOR OUTSIDE OF libclass_loader WITHIN THE PLUGIN LIBRARY! T
+   * @brief Default virtual destructor
    */
-  ~AbstractMetaObjectBase();
+  virtual ~AbstractMetaObjectBase() = default;
 
   /**
    * @brief Gets the literal name of the class.
@@ -144,12 +143,13 @@ class CLASS_LOADER_PUBLIC AbstractMetaObjectBase
   ClassLoader * getAssociatedClassLoader(size_t index) const;
 
 protected:
-  /**
-   * This is needed to make base class polymorphic (i.e. have a vtable)
-   */
-  virtual void dummyMethod() {}
+  typedef std::vector<class_loader::ClassLoader *> ClassLoaderVector;
 
-  AbstractMetaObjectBaseImpl * impl_;
+  ClassLoaderVector associated_class_loaders_;
+  std::string associated_library_path_;
+  std::string base_class_name_;
+  std::string class_name_;
+  std::string typeid_base_class_name_;
 };
 
 /**