Skip to content

fix(Executor): Fix segfault if callback group is deleted during rmw_wait#2682

Merged
fujitatomoya merged 1 commit into
ros2:jazzyfrom
cellumation:seg_fault_fix
Dec 1, 2024
Merged

fix(Executor): Fix segfault if callback group is deleted during rmw_wait#2682
fujitatomoya merged 1 commit into
ros2:jazzyfrom
cellumation:seg_fault_fix

Conversation

@jmachowinski
Copy link
Copy Markdown
Collaborator

@jmachowinski jmachowinski commented Nov 22, 2024

Fixes #2664

@alsora @mjcarroll A review please

@msplr Can you confirm that this fixes the issue ?

christophebedard
christophebedard reviewed Nov 22, 2024
View reviewed changes
Comment thread rclcpp/src/rclcpp/executor.cpp Outdated
wjwwood
wjwwood reviewed Nov 22, 2024
View reviewed changes
Comment thread rclcpp/src/rclcpp/executor.cpp Outdated
wjwwood
wjwwood approved these changes Nov 22, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with style fixups

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Nov 22, 2024

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.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Nov 22, 2024

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

fujitatomoya
fujitatomoya reviewed Nov 22, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmachowinski @wjwwood

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?

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Nov 22, 2024

That's also fine with me, I'm not sure about the urgency.

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Nov 23, 2024

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,
and your framework just segfaults on you. Therefore I want to get a fix merged ASAP. Sadly, we just missed the jazzy sync for the binaries by 2 days...

@msplr
Copy link
Copy Markdown

msplr commented Nov 23, 2024

@msplr Can you confirm that this fixes the issue ?

I confirm that it fixes the segfault in my sample code, thanks!

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Nov 23, 2024

@jmachowinski that is true, and we just missed it... so probably what we do here is,

@msplr thanks!

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Nov 25, 2024

Added a test, fixed the same bug in the StaticSingleThreadedExecutor....

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Nov 25, 2024 

* and backport [fix(Executor): Fix segfault if callback group is deleted during rmw_wait #2683](https://github.com/ros2/rclcpp/pull/2683) to jazzy.

This PR is the backport of #2683 to Jazzy...

fix(Executor): Fix segfault if callback group is deleted during rmw_wait
47ee257 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Nov 28, 2024

@alsora @ahcorde Can one of you start a build pipeline ?

fujitatomoya
fujitatomoya approved these changes Nov 30, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. i will start the CI.

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Nov 30, 2024

Pulls: #2682
Gist: https://gist.githubusercontent.com/fujitatomoya/112f5fd7e91ce9c0c5644e5924f504f7/raw/b34c4a7a4636e6fe3225e7dba4a05536efcccac9/ros2.repos
BUILD args: --packages-above-and-dependencies rclcpp
TEST args: --packages-above rclcpp
ROS Distro: jazzy
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14876

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit df09c6e into ros2:jazzy Dec 1, 2024
@fujitatomoya fujitatomoya mentioned this pull request Dec 1, 2024
Closed
@KarthickCK-lam
Copy link
Copy Markdown

KarthickCK-lam commented Jan 30, 2026

can this be back ported to humble as well ?

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Jan 30, 2026

This should not affect humble, as the bug was introduced by a refactoring between humble and jazzy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christophebedard christophebedard christophebedard left review comments

@wjwwood wjwwood wjwwood approved these changes

@mjcarroll mjcarroll mjcarroll approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

@hidmic hidmic Awaiting requested review from hidmic

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@jmachowinski @wjwwood @msplr @fujitatomoya @KarthickCK-lam @mjcarroll @christophebedard