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.

Sign up for GitHub

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

Jump to bottom

fix nullptr dereference in prune_requests_older_than #2008

Merged
merged 3 commits into from
Dec 13, 2022
Merged

fix nullptr dereference in prune_requests_older_than #2008

merged 3 commits into from
Dec 13, 2022

Conversation

akela1101
Copy link
Contributor

@akela1101 akela1101 commented Aug 30, 2022 •
edited
Loading

Fixes #2007

@akela1101 akela1101 changed the title #2007 fix nullptr dereference in prune_requests_older_than fix nullptr dereference in prune_requests_older_than Aug 30, 2022
clalancette
clalancette requested changes Aug 30, 2022
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This function is buggy as you point out, so thanks for the fix.

That said, the reason we didn't know it was wrong was that we don't have a test for this function. Could you add two tests, one of which has pruned_requests as nullptr (or just not passed), and one of which has pruned_requests as a pointer?

(also, completely orthogonally, why is pruned_requests a pointer and not a reference? Maybe @ivanpauno can chime in since he added this API)

@ivanpauno
Copy link
Member

(also, completely orthogonally, why is pruned_requests a pointer and not a reference? Maybe @ivanpauno can chime in since he added this API)

It's an optional output argument, so the vector isn't used if nullptr is provided.
References cannot be nullptr, so they cannot be used in this case.
For some reason I forgot the obvious if() in the original PR.

@clalancette clalancette added the more-information-needed Further information is required label Sep 15, 2022
@akela1101 akela1101 force-pushed the patch-1 branch 2 times, most recently from 7176087 to 37c6c0b Compare November 23, 2022 11:18
@akela1101
Copy link
Contributor Author

@clalancette Sorry for waiting. I tried to add 2 tests, please check.

The way I tested them locally was:

colcon build --packages-select rclcpp
colcon test --packages-select rclcpp --ctest-args -R test_client
colcon test-result --verbose

ivanpauno
ivanpauno approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

lgtm!

rclcpp/test/rclcpp/test_client.cpp Outdated Show resolved Hide resolved
rclcpp/test/rclcpp/test_client.cpp Outdated Show resolved Hide resolved
@akela1101
Copy link
Contributor Author

1u

nice suggestion, fixed

@fujitatomoya
Copy link
Collaborator

CI:

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

@ivanpauno
Copy link
Member

  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

Windows CI is not working yet...

@akela1101 can you address DCO failure?

@akela1101
Update rclcpp/test/rclcpp/test_client.cpp
b6c9723 
Co-authored-by: Chen Lihui <[email protected]>
Signed-off-by: akela1101 <[email protected]>
@ivanpauno
Copy link
Member

  • Windows Build Status

@ivanpauno ivanpauno merged commit 1ac37b6 into ros2:rolling Dec 13, 2022
@ivanpauno
Copy link
Member

@Mergifyio backport humble

@mergify mergify bot mentioned this pull request Dec 13, 2022
Merged
@mergify
Copy link
Contributor

mergify bot commented Dec 13, 2022

backport humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Dec 13, 2022
@akela1101 @mergify
fix nullptr dereference in prune_requests_older_than (#2008)
d79702e 
* fix nullptr dereference in prune_requests_older_than

Signed-off-by: akela1101 <[email protected]>

* add tests for prune_requests_older_than

Signed-off-by: akela1101 <[email protected]>

* Update rclcpp/test/rclcpp/test_client.cpp

Co-authored-by: Chen Lihui <[email protected]>
Signed-off-by: akela1101 <[email protected]>

Signed-off-by: akela1101 <[email protected]>
Co-authored-by: Chen Lihui <[email protected]>
(cherry picked from commit 1ac37b6)
@akela1101 akela1101 deleted the patch-1 branch December 13, 2022 14:32
ivanpauno pushed a commit that referenced this pull request Dec 13, 2022
@mergify @akela1101
fix nullptr dereference in prune_requests_older_than (#2008) (#2065)
f9050cd 
* fix nullptr dereference in prune_requests_older_than

Signed-off-by: akela1101 <[email protected]>

* add tests for prune_requests_older_than

Signed-off-by: akela1101 <[email protected]>

* Update rclcpp/test/rclcpp/test_client.cpp

Co-authored-by: Chen Lihui <[email protected]>
Signed-off-by: akela1101 <[email protected]>

Signed-off-by: akela1101 <[email protected]>
Co-authored-by: Chen Lihui <[email protected]>
(cherry picked from commit 1ac37b6)

Co-authored-by: andrei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iuhilnehc-ynos iuhilnehc-ynos iuhilnehc-ynos left review comments

@clalancette clalancette clalancette requested changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

@alsora alsora alsora approved these changes

@hidmic hidmic Awaiting requested review from hidmic hidmic is a code owner

@wjwwood wjwwood Awaiting requested review from wjwwood wjwwood is a code owner

Assignees
No one assigned
Labels
more-information-needed Further information is required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SEGFAULT in prune_requests_older_than() if pruned_requests are not provided
6 participants
@akela1101 @ivanpauno @fujitatomoya @clalancette @alsora @iuhilnehc-ynos