Fix lock order inversion in rmw_wait#1005
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
jmachowinski
changed the title
Yadunund
left a comment
Yadunund
left a comment
There was a problem hiding this comment.
@jmachowinski thanks for looking into the issue and providing a fix. I'll review the changes more closely but left an early comment for your consideration.
| @@ -1,97 +0,0 @@ | |||
| // Copyright 2026 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
Can we retain + update the test to capture the issue faced during rosbag record? It would be great to have a way to check against the deadlock behavior and to prevent future regressions.
There was a problem hiding this comment.
We could hammer a guard condition in an endless loop in a thread and wait a few times in a second thread that should trigger the dead lock.
There was a problem hiding this comment.
That works. Do you have a way for me to reproduce the use after free issue to test both cases against this PR?
There was a problem hiding this comment.
Same test, just delete the waitset directly after the rcl_wait. This is basically what the executor will do.
I would not put the test in this pkg, but rather in to rcl or system_tests. I was also looking at the fastrtps impl for rmw_wait and it is almost identical. Therefore having a test in a unified location checking both makes more sense to me.
There was a problem hiding this comment.
For clarification on the use after free :
If this code runs in thread 1:
rmw_wait_set_data_t * wait_set_data_to_trigger = nullptr;
{
std::lock_guard<std::mutex> lock(internal_mutex_);
// the change to hasTriggered_ needs to be mutually exclusive with
// rmw_wait() which checks hasTriggered() and decides if wait() needs to
// be called
has_triggered_ = true;
wait_set_data_to_trigger = wait_set_data_;
}Now we switch to Thread 0 and return from rcl_wait and delete the waitset.
Switch back to Thread 1
// now wait_set_data_to_trigger points to deleted memory
if (wait_set_data_to_trigger != nullptr) {
std::lock_guard<std::mutex> wait_set_lock(wait_set_data_to_trigger->condition_mutex);
wait_set_data_to_trigger->triggered = true;
wait_set_data_to_trigger->condition_variable.notify_one();
}This is seriously racy, and the only chance for creating a test case is hammering the trigger from a second thread. And even then there is a chance that you don't catch the bug. These things are hard to test and I don't know if it is worth the effort to try to create a test.
|
@jmachowinski |
|
Pulls: #1005
|
|
Pulls: #1005
|
|
TL;DR: Same as @araitaiga , we believe this patch fixes the issue we have observed (Dockerised Jazzy Ubuntu 24.04). For reference, we are running data recording on an AV with many Docker containers, 100+ topics (inc. chonky camera images & lidar pointclouds), about ~1GB/s total. I'll post back if we see this issue appear again while using this patch. In any case, thank you very much for your work!! |
|
@Yadunund The CI seems as green as you can get it if actually running the system tests with zenoh and not fastdds. I would like to merge this now and start a backport so that we can get it in the lyrical sync. @MiguelCompany added a test here ros2/rmw_implementation#280 that should catch this issue as well. So I don't see the point of adding another one in this repo. |
|
@jmachowinski mind resolving the merge conflict? |
* Fix runtime msg dependencies for runtime script * Add convenient script to run ghcr.io docker image * Update Dockerfile with msg pkgs available upstream - pandar_msgs and autoware_msgs now (finally) available via apt - Fix tiny typo * Fix pre-commit issues * Apply temporary patch to fix rmw_zenoh freeze - ros2/rmw_zenoh#1005 * Fix pre-commit issues
This fixes a deadlock if a guard condition was triggered while a guard condition was being attached or detached. Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
c6f8e44 to
73a6254
Compare
|
Pulls: #1005
|
|
@jmachowinski @Yadunund Thanks for fixing this. Are there plans to backport this fix to Jazzy, Kilted, and Lyrical? |
|
Yeah, but I need to do it manually |
|
Thanks for the quick great work everyone! 🎉 |
|
@GreatAlexander will get it out in the next day or so. |
|
Thank you for backporting the patch! @jmachowinski @Yadunund |
5 participants
Description
This is a second attempted to fix #921
From my point of view the previous patches introduced in
#937 and #992
introduced a use after free issue.
If the triggering thread is scheduled away after obtaining the wait_set_data_
pointer, another thread may delete the waitset directly afterwards.
Then the locally saved pointer points to invalid memory.
Fixes #921
Is this user-facing behavior change?
No deadlocks any more ?
Did you use Generative AI?
I asked AI up front and it came up with the broken patch from #937
This one was written by me...
Additional Information
We hit this bug on shutdown of ros2 bag record.