fix(Executor): Fix segfault if callback group is deleted during rmw_wait #2682
Conversation
jmachowinski
requested review from
ivanpauno,
hidmic and
wjwwood
as code owners
rclcpp/src/rclcpp/executor.cpp
Outdated
| for(const auto &w_ptr : callback_groups) | ||
| { | ||
| auto shr_ptr = w_ptr.lock(); | ||
| if(shr_ptr) | ||
| { |
There was a problem hiding this comment.
| for(const auto &w_ptr : callback_groups) | |
| { | |
| auto shr_ptr = w_ptr.lock(); | |
| if(shr_ptr) | |
| { | |
| for (const auto & w_ptr : callback_groups) { | |
| auto shr_ptr = w_ptr.lock(); | |
| if (shr_ptr) { |
rclcpp/src/rclcpp/executor.cpp
Outdated
| } | ||
| this->wait_result_.emplace(wait_set_.wait(timeout)); | ||
| if (!this->wait_result_ || this->wait_result_->kind() == WaitResultKind::Empty) { | ||
| RCUTILS_LOG_WARN_NAMED( | ||
| "rclcpp", | ||
| "empty wait set received in wait(). This should never happen."); | ||
| } else { | ||
| // drop references to the callback groups, before trying to execute anything | ||
| cbgs.clear(); |
There was a problem hiding this comment.
Just to check my understanding we don't need something like this in the if-clause because cbgs is on the stack and will be destroyed when exiting, right?
There was a problem hiding this comment.
yes, that is the idea.
At this point we need to explicitly drop the shared pointers, to make sure, we don't get callbacks by the 'execute' into something that was already dropped by 'userland'
jmachowinski
force-pushed
the
seg_fault_fix
branch
from
7795931
to
009a7b0
Compare
|
I think we should push to get this fix in, but it would be nice to have a regression test for this case if we can manage it. I know it's hard with races like this, but still it would be good. |
|
CI:
|
There was a problem hiding this comment.
maybe we can work on #2683 1st and then backport the fix to jazzy including unit tests? it's been there already for a while, i think we can go along with normal procedure?
|
That's also fine with me, I'm not sure about the urgency. |
I was not aware of this bug until yesterday. To me, this is is worst bug from a user perspective. You do an allowed operation, |
I confirm that it fixes the segfault in my sample code, thanks! |
|
@jmachowinski that is true, and we just missed it... so probably what we do here is,
@msplr thanks! |
Signed-off-by: Janosch Machowinski <[email protected]>
jmachowinski
force-pushed
the
seg_fault_fix
branch
from
009a7b0
to
e5f6dcc
Compare
|
Added a test, fixed the same bug in the StaticSingleThreadedExecutor.... |
This PR is the backport of #2683 to Jazzy... |
Fixes #2664
@alsora @mjcarroll A review please
@msplr Can you confirm that this fixes the issue ?