From e7e314dc62a85609b4a9c45062b6fba2c3acf5a4 Mon Sep 17 00:00:00 2001 From: Tamaki Nishino Date: Thu, 27 Aug 2026 01:02:03 +0900 Subject: [PATCH] Make loadLibrary()/unloadLibrary() safe against concurrent load and unload loadLibrary() locked only its own dlopen() and the final push_back onto the loaded library vector, and unloadLibrary() took no lock at all while it erased from that same vector and dropped the last reference to the shared library. Two threads loading and unloading the same plugin therefore race: the loader either fails to find the class and throws MultiLibraryClassLoader: Could not create class of type because the metaobjects were destroyed between the isLibraryLoadedByAnybody() check and binding the new owner, or it segfaults inside the dlclose() path. Hold one recursive mutex across the whole of both functions so a dlopen() can never overlap a dlclose() of the same library, take the loaded library vector mutex around the erase in unloadLibrary(), and guard the already-loaded branch of loadLibrary() with the factory map mutex. The last two hunks restore what #40 did on indigo-devel in 2016; that fix reached melodic-devel and noetic-devel but was never forward-ported to the ROS 2 branches, which still carry the 2014 locking. See #39 and ros2/rosbag2#2350. Signed-off-by: Tamaki Nishino --- src/class_loader_core.cpp | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/class_loader_core.cpp b/src/class_loader_core.cpp index e808953..55c475d 100644 --- a/src/class_loader_core.cpp +++ b/src/class_loader_core.cpp @@ -56,6 +56,16 @@ std::recursive_mutex & getPluginBaseToFactoryMapMapMutex() return m; } +namespace +{ +// Serializes loadLibrary()/unloadLibrary() so a dlopen() never overlaps a dlclose(). +std::recursive_mutex & getLoaderMutex() +{ + static std::recursive_mutex m; + return m; +} +} // namespace + BaseToFactoryMapMap & getGlobalPluginBaseToFactoryMapMap() { static BaseToFactoryMapMap instance; @@ -390,6 +400,8 @@ void purgeGraveyardOfMetaobjects( void loadLibrary(const std::string & library_path, ClassLoader * loader) { + std::lock_guard loader_lock(getLoaderMutex()); + CONSOLE_BRIDGE_logDebug( "class_loader.impl: " "Attempting to load library %s on behalf of ClassLoader handle %p...\n", @@ -397,6 +409,7 @@ void loadLibrary(const std::string & library_path, ClassLoader * loader) // If it's already open, just update existing metaobjects to have an additional owner. if (isLibraryLoadedByAnybody(library_path)) { + std::lock_guard lock(getPluginBaseToFactoryMapMapMutex()); CONSOLE_BRIDGE_logDebug( "%s", "class_loader.impl: " @@ -407,11 +420,7 @@ void loadLibrary(const std::string & library_path, ClassLoader * loader) std::shared_ptr library_handle; - static std::recursive_mutex loader_mutex; - { - std::lock_guard loader_lock(loader_mutex); - setCurrentlyActiveClassLoader(loader); setCurrentlyLoadingLibraryName(library_path); try { @@ -463,6 +472,8 @@ void loadLibrary(const std::string & library_path, ClassLoader * loader) void unloadLibrary(const std::string & library_path, ClassLoader * loader) { + std::lock_guard loader_lock(getLoaderMutex()); + if (hasANonPurePluginLibraryBeenOpened()) { CONSOLE_BRIDGE_logDebug( "class_loader.impl: " @@ -479,6 +490,7 @@ void unloadLibrary(const std::string & library_path, ClassLoader * loader) "Unloading library %s on behalf of ClassLoader %p...", library_path.c_str(), reinterpret_cast(loader)); + std::lock_guard llv_lock(getLoadedLibraryVectorMutex()); LibraryVector & open_libraries = getLoadedLibraryVector(); LibraryVector::iterator itr = findLoadedLibrary(library_path); if (itr != open_libraries.end()) {