Skip to content

fix nullptr dereference in prune_requests_older_than#2008

Merged
ivanpauno merged 3 commits intoros2:rollingfrom
akela1101:patch-1
Dec 13, 2022
Merged

fix nullptr dereference in prune_requests_older_than#2008
ivanpauno merged 3 commits intoros2:rollingfrom
akela1101:patch-1

Conversation

@akela1101
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

ivanpauno commented Aug 30, 2022

(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
fix nullptr dereference in prune_requests_older_than
bb178d6 
Signed-off-by: akela1101 <akela1101@gmail.com>
@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
Copy Markdown
Contributor Author

akela1101 commented Nov 23, 2022

@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
Copy Markdown
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!

@akela1101
add tests for prune_requests_older_than
4ddbfbc 
Signed-off-by: akela1101 <akela1101@gmail.com>
@akela1101
Copy link
Copy Markdown
Contributor Author

akela1101 commented Nov 23, 2022

1u

nice suggestion, fixed

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Nov 30, 2022

CI:

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

iuhilnehc-ynos
iuhilnehc-ynos reviewed Dec 1, 2022
View reviewed changes
@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Dec 1, 2022

  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Dec 1, 2022

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 <lihui.chen@sony.com>
Signed-off-by: akela1101 <akela1101@gmail.com>
@ivanpauno
Copy link
Copy Markdown
Member

ivanpauno commented Dec 6, 2022

  • Windows Build Status

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

ivanpauno commented Dec 13, 2022

@Mergifyio backport humble

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

mergify bot commented Dec 13, 2022

backport humble

✅ Backports have been created

Details

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 <akela1101@gmail.com>

* add tests for prune_requests_older_than

Signed-off-by: akela1101 <akela1101@gmail.com>

* Update rclcpp/test/rclcpp/test_client.cpp

Co-authored-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: akela1101 <akela1101@gmail.com>

Signed-off-by: akela1101 <akela1101@gmail.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(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 <akela1101@gmail.com>

* add tests for prune_requests_older_than

Signed-off-by: akela1101 <akela1101@gmail.com>

* Update rclcpp/test/rclcpp/test_client.cpp

Co-authored-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: akela1101 <akela1101@gmail.com>

Signed-off-by: akela1101 <akela1101@gmail.com>
Co-authored-by: Chen Lihui <lihui.chen@sony.com>
(cherry picked from commit 1ac37b6)

Co-authored-by: andrei <akela1101@gmail.com>
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

@alsora alsora alsora approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

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