Fix data race on AbstractMetaObjectBase's owning-loader list by thomasmoore16 · Pull Request #236 · ros/class_loader

thomasmoore16 · 2026-09-21T20:54:01Z

Problem

AbstractMetaObjectBase::addOwningClassLoader() / removeOwningClassLoader() / isOwnedBy() (and the other accessors of associated_class_loaders_) take no lock of their own. createInstance() in class_loader_core.hpp releases getPluginBaseToFactoryMapMapMutex() before reading factory->isOwnedBy(loader), so a concurrent addOwningClassLoader() / removeOwningClassLoader() on another thread (e.g. from loadLibrary()'s already-loaded branch) can race with that read — and, since it's a std::vector, with any other concurrent read/write of the same metaobject's owner list.

I hit this with ThreadSanitizer as a data race on std::vector<ClassLoader *>'s push_back()/erase() (via the vector's internal reallocation). It's straightforward to trigger 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, all for the first time, simultaneously.

This is separate from #234 (loadLibrary()/unloadLibrary() overlap) and #231 (has_unmanaged_instance_been_created_) — both already merged, and both fix different unsynchronized globals in this same area. I checked the current rolling HEAD before writing this: createInstance() still releases the mutex before the isOwnedBy() checks, and meta_object.cpp's accessors are still fully unguarded.

Fix

Give AbstractMetaObjectBaseImpl its own recursive_mutex guarding associated_class_loaders_, so every accessor (addOwningClassLoader, removeOwningClassLoader, isOwnedBy, isOwnedByAnybody, getAssociatedClassLoadersCount, getAssociatedClassLoader) 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: between releasing that mutex (previously done right after the map lookup) and reading isOwnedBy(), the factory pointer looked up under it could in principle be invalidated by a concurrent destroyMetaObjectsForLibrary() on another thread. The mutex-per-vector fix above closes the vector-corruption race on its own; this closes the separate use-after-free risk on the factory pointer itself.

Testing

Added ClassLoaderTest.threadSafetyMultipleLoadersPerLibrary to test/utest.cpp. Unlike the existing ClassLoaderTest.threadSafety (one ClassLoader shared by all STRESS_TEST_NUM_THREADS threads, so isOwnedBy() is always queried with the loader that already, and permanently, owns the metaobject), this constructs a separate ClassLoader per thread for the same library, so construction/destruction concurrently mutates associated_class_loaders_ while other threads concurrently read it via isOwnedBy() in createInstance(). An anchor ClassLoader is kept alive for the whole test so the library's owner count never reaches zero and the worker threads never trigger the actual unload/dlclose() path — that path has its own pre-existing, unrelated refcounting bug (a library can be dlclose()-d twice when independent ClassLoaders for the same library race to be "last owner out"), which isn't what this test is targeting and would otherwise fail it intermittently regardless of this fix.

I built and ran this locally against a ros:rolling-ros-base container (colcon, -DBUILD_TESTING=ON), both with a plain build and with ThreadSanitizer. Unpatched, ThreadSanitizer reliably reports the addOwningClassLoader()/removeOwningClassLoader() race on associated_class_loaders_ (9-10 reports per run); with the fix, zero reports across many runs. The full colcon test --packages-select class_loader suite passes 100% (9/9) with the fix applied.

Generative AI

This fix was investigated, written, and iteratively verified (ThreadSanitizer, stress-testing, and a local colcon/ROS 2 rolling build) by Claude (Anthropic), operating as Claude Code, at the direction of and reviewed by me. The regression test and this PR description were also written by Claude; I reviewed both before submitting.

thomasmoore16 · 2026-09-22T14:01:45Z

For reference, tracking the two related PRs mentioned above, both already merged:

Make loadLibrary()/unloadLibrary() safe against concurrent load and unload #234 (loadLibrary()/unloadLibrary() concurrent load/unload): 221ae79
concurrent accesses well-defined #231 (has_unmanaged_instance_been_created_ atomic): ae7ea8a

This PR fixes a third, still-unaddressed race in the same area (AbstractMetaObjectBase's owning-loader list).

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>

The regression test constructed and destroyed STRESS_TEST_NUM_THREADS (500) independent ClassLoaders for the same library with no synchronization between their lifetimes, so the library's refcount could repeatedly bounce to zero under contention, forcing many real dlopen()/dlclose() cycles - and occasionally racing two of those "last owner out" unloads against each other, an existing, separate bug in this library's load/unload refcounting unrelated to the fix under test here. Add an `anchor` ClassLoader kept alive for the whole test so the refcount never reaches zero and that unrelated path is never exercised, isolating the associated_class_loaders_ race this fix actually targets. Verified in a local colcon/ROS 2 rolling Docker environment with ThreadSanitizer: unpatched, this reliably reports the addOwningClassLoader()/removeOwningClassLoader() race (9-10 reports per run); with the fix, zero reports across many runs, and the full `colcon test` suite for class_loader passes 100% (9/9). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: thomasmoore-torc <thomas.moore@torc.ai>

mjcarroll

Thanks for tracking this down. I have some feedback here.

mjcarroll · 2026-11-02T20:09:41Z

+      // isOwnedBy() must be read while still holding this mutex: another thread's
+      // addOwningClassLoader()/removeOwningClassLoader() (e.g. from loadLibrary()'s
+      // already-loaded branch) is only serialized against this same mutex.


This comment says add/remove are "only serialized against this same mutex", but this PR also adds a per-object mutex in meta_object.cpp, which contradicts it.

mjcarroll · 2026-11-02T20:11:16Z

  Base * obj = nullptr;
-  if (factory != nullptr && factory->isOwnedBy(loader)) {
+  if (factory != nullptr && owned_by_loader) {
    obj = factory->create(std::forward<Args>(args)...);


Reading isOwnedBy() under the lock is an improvement, but this doesn't close the use-after-free described in the PR. factory->create() still runs after the map mutex is released. If another thread drops the last owner in that window, destroyMetaObjectsForLibrary moves the metaobject to the graveyard and the library can be dlclose'd. We'd then call create() on a freed metaobject. owned_by_loader is also out of date by this point.

Could we keep the lock held through create()?

mjcarroll · 2026-11-02T20:11:53Z

 {
 public:
+  // Guards associated_class_loaders_: class_loader_core.hpp's createInstance() reads
+  // isOwnedBy() after releasing getPluginBaseToFactoryMapMapMutex(), so this vector needs


After this PR, createInstance() no longer reads isOwnedBy() after releasing etPluginBaseToFactoryMapMapMutex(). This comment describes the old code?

mjcarroll · 2026-11-02T20:13:26Z

+  // Guards associated_class_loaders_: class_loader_core.hpp's createInstance() reads
+  // isOwnedBy() after releasing getPluginBaseToFactoryMapMapMutex(), so this vector needs
+  // its own lock independent of that outer mutex.
+  mutable std::recursive_mutex associated_class_loaders_mutex_;


Every writer of associated_class_loaders_ already holds getPluginBaseToFactoryMapMapMutex(), the actual bug is readers that run outside of that mutex. The createInstacne change already fixes the one in this PR. I'd rather drop the per-object mutex and have the remaining readers use the map mutex.

I think that a regular mutex is also sufficient, since nothing should re-enter the lock?

mjcarroll · 2026-11-02T20:14:50Z


 ClassLoader * AbstractMetaObjectBase::getAssociatedClassLoader(size_t index) const
 {
+  std::lock_guard<std::recursive_mutex> lock(impl_->associated_class_loaders_mutex_);


This doesn't necessarily make it safe.

printDebugInfoToScreen calls getAssociatedClassLoadersCount() and then loops over getAssociatedClassLoader(i). A concurrent remove would make index read past the end.

mjcarroll · 2026-11-02T20:15:44Z

+      [&failures]() {
+        try {
+          class_loader::ClassLoader loader(LIBRARY_1);
+          std::vector<std::string> classes = loader.getAvailableClasses<Base>();


I think that we want to assert that classes isn't empty so that we know we are actually testing something?

mjcarroll · 2026-11-02T20:16:24Z

+            loader.createInstance<Base>(class_name)->saySomething();
+          }
+        } catch (...) {
+          ++failures;


Can we actually catch the exception and print it here so that we can get something if there is a failure?

thomasmoore-torc and others added 2 commits September 22, 2026 16:03

thomasmoore16 force-pushed the fix/isowned-by-race-and-createinstance-early-unlock branch from 80ed16e to 18bc709 Compare September 22, 2026 16:04

mjcarroll reviewed Nov 2, 2026

View reviewed changes

Navigation Menu

Repository navigation

Fix data race on AbstractMetaObjectBase's owning-loader list - #236

Fix data race on AbstractMetaObjectBase's owning-loader list#236
thomasmoore16 wants to merge 2 commits into
ros:rollingfrom
thomasmoore16:fix/isowned-by-race-and-createinstance-early-unlock

thomasmoore16 commented Sep 21, 2026 •

edited

Loading

Uh oh!

thomasmoore16 commented Sep 22, 2026

Uh oh!

mjcarroll left a comment

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

mjcarroll Nov 2, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

thomasmoore16 commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Testing

Generative AI

Uh oh!

thomasmoore16 commented Sep 22, 2026

Uh oh!

mjcarroll left a comment

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

mjcarroll Nov 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thomasmoore16 commented Sep 21, 2026 •

edited

Loading