Repository navigation
Make loadLibrary()/unloadLibrary() safe against concurrent load and unload - #234
Merged
clalancette merged 1 commit intoSep 2, 2026
Conversation
…nload 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 <T> 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 ros#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 ros#39 and ros2/rosbag2#2350. Signed-off-by: Tamaki Nishino <otamachan@gmail.com>
clalancette
approved these changes
Sep 1, 2026
clalancette
left a comment
clalancette
left a comment
Contributor
There was a problem hiding this comment.
Seems reasonable to me. I'll run CI on it next.
Contributor
|
CI:
|
thomasmoore-torc
added a commit
to thomasmoore-torc/class_loader
that referenced
this pull request
Sep 22, 2026
AbstractMetaObjectBase::addOwningClassLoader()/removeOwningClassLoader()/ isOwnedBy() (and the other accessors of associated_class_loaders_) took no lock of their own. createInstance() in class_loader_core.hpp released getPluginBaseToFactoryMapMapMutex() before reading factory->isOwnedBy(loader), so a concurrent addOwningClassLoader()/removeOwningClassLoader() on another thread (e.g. from loadLibrary()'s already-loaded branch, or from onPluginDeletion()) could race with that read - and, since it's a std::vector, with any other concurrent read/write of the same metaobject's owner list. Reported by ThreadSanitizer as a data race on std::vector<ClassLoader *>'s push_back()/erase() (via vector's internal reallocation), hit by constructing many independent ClassLoader instances for the same library concurrently - e.g. one rosbag2_cpp::Writer per thread, each internally constructing its own pluginlib::ClassLoader for the serialization format converter. Give AbstractMetaObjectBaseImpl its own recursive_mutex guarding associated_class_loaders_, so every accessor is self-contained and safe regardless of what lock (if any) the caller happens to be holding. Also extend createInstance()'s existing getPluginBaseToFactoryMapMapMutex() scope to cover the isOwnedBy() checks themselves, since between releasing that mutex and reading isOwnedBy(), the factory pointer looked up under it could in principle be invalidated by a concurrent destroyMetaObjectsForLibrary() on another thread. This is separate from ros#234 (loadLibrary()/unloadLibrary() overlap) and ros#231 (has_unmanaged_instance_been_created_): those fixed different unsynchronized globals in this same area. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: thomasmoore-torc <thomas.moore@torc.ai>
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
loadLibrary()andunloadLibrary()can run concurrently on the same library:loadLibrary()locks only its owndlopen()and the finalpush_backonto the loadedlibrary vector. The
isLibraryLoadedByAnybody()test at the top is a check-then-act.unloadLibrary()takes no lock at all while iterase()s from that same vector and dropsthe last reference to the shared library.
Two threads therefore mutate one
std::vectorconcurrently, and adlopen()can overlap adlclose(). That shows up either aswhen the metaobjects go away between the check and the bind, or as a SIGSEGV in
unloadLibrary()->rcpputils::SharedLibrary::unload_library()while another thread isinside
_dl_map_object_from_fd()for the same plugin.ros2/rosbag2#2350 is hitting this. We hit it in production on Jazzy, where each
rosbag2_cpp::Writerowns aStorageFactoryand so loads and unloads the mcap storageplugin once per bag.
Change
Hold one recursive mutex across the whole of
loadLibrary()andunloadLibrary(), takegetLoadedLibraryVectorMutex()around the erase, and guard the already-loaded branch withgetPluginBaseToFactoryMapMapMutex(). Lock order is loader mutex -> vector / factory mapeverywhere, and the loader mutex is recursive so the plugin registration that runs from
inside
dlopen()on the same thread is fine.The last two hunks restore #40, which fixed #39 on
indigo-develin 2016 and reachedmelodic-develandnoetic-develbut never the ROS 2 branches — those still carry the 2014locking from 96ed2e4. #40 only touched the load side, so locking
unloadLibrary()is neededon top of that forward-port.
Verification
Two threads doing nothing but open/close an MCAP bag through
rosbag2_cpp::Writeron Jazzy.Only
libclass_loader.sodiffers between rows.Soak on the patched build: 8 threads x 400, 3 runs, clean. Over 200 fresh processes each
doing one concurrent open per thread: 12/200 SIGSEGV before, 0/200 after. Single-threaded
behaviour is unchanged.