Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix expired goals capacity of action server #931

Merged
merged 1 commit into from
Sep 14, 2021
Merged

fix expired goals capacity of action server #931

merged 1 commit into from
Sep 14, 2021

Conversation

spiralray
Copy link
Contributor

fix #926

This fix is important for action server nodes especially on Windows.

Signed-off-by: Kentaro Tanaka <spiralray@gmail.com>
@fujitatomoya fujitatomoya self-requested a review September 1, 2021 15:51
@spiralray
Copy link
Contributor Author

spiralray commented Sep 5, 2021

PROBLEM

If more than one goals received, buffer-overflow occurs at the variable rcl_action_expire_goals when some messages get expired.

DETAILS

The method rcl_action_expire_goals checks whether action goals are expired, and expired goals are copied to the buffer expired_goals.
The parameter expired_goals_capacity shows the size of the buffer expired_goals.

At line 605, variable i indicates the current for-loop index of the goal_handle which is checked expired or not.
And variable 'i' is decerment when the targeted goal_handle is expired and deleted from the array action_server->impl->goal_handles.

So, if expired_goals_capacity equals one and action_server->impl->goal_handles[0] get expired, buffer overflow will occur.

よろしくお願いします。

@fujitatomoya fujitatomoya added the bug Something isn't working label Sep 7, 2021
Copy link
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

@fujitatomoya
Copy link
Collaborator

https://build.ros2.org/job/Rpr__rcl__ubuntu_focal_amd64/579/ are unrelated to the fix.

@ros-pull-request-builder retest this please

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@KavenYau
Copy link

@fujitatomoya @clalancette Would this be backported to foxy?

@fujitatomoya
Copy link
Collaborator

@Mergifyio backport foxy galactic

mergify bot pushed a commit that referenced this pull request Jan 12, 2022
Signed-off-by: Kentaro Tanaka <spiralray@gmail.com>
(cherry picked from commit eae50c9)
mergify bot pushed a commit that referenced this pull request Jan 12, 2022
Signed-off-by: Kentaro Tanaka <spiralray@gmail.com>
(cherry picked from commit eae50c9)
@mergify
Copy link

mergify bot commented Jan 12, 2022

backport foxy galactic

✅ Backports have been created

jacobperron pushed a commit that referenced this pull request Feb 4, 2022
Signed-off-by: Kentaro Tanaka <spiralray@gmail.com>
(cherry picked from commit eae50c9)

Co-authored-by: spiralray <spiralray@users.noreply.github.com>
clalancette pushed a commit that referenced this pull request Apr 28, 2022
Signed-off-by: Kentaro Tanaka <spiralray@gmail.com>
(cherry picked from commit eae50c9)

Co-authored-by: spiralray <spiralray@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expiring multiple action results causes crash
4 participants