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

memmove for overlaping memory #434

Merged
merged 3 commits into from
Nov 4, 2023
Merged

memmove for overlaping memory #434

merged 3 commits into from
Nov 4, 2023

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Nov 3, 2023

Fixes #433

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 3, 2023

I'm unsure what to do next. I assume some huge CI job needs to run to test this?

@tylerjw
memmove for overlaping memory
dfd0705 
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw tylerjw force-pushed the memcpy-param-overlap branch from f55e769 to dfd0705 Compare November 3, 2023 15:46
@clalancette
Copy link
Contributor

I'm unsure what to do next. I assume some huge CI job needs to run to test this?

Well, the commit needs a Signed-off-by line, and I think it would be good to have a test exercising this. But after that, and review, yes, it needs full CI (which one of the maintainers will run).

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 3, 2023

Is there a way to add testing this with asan (as that is how we found this)?

@clalancette
Copy link
Contributor

Is there a way to add testing this with asan (as that is how we found this)?

Theoretically, https://ci.ros2.org/view/nightly/job/nightly_linux_address_sanitizer/ should catch this. So if there is a test that exercises it, it should show up there.

@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 3, 2023

Added the signed-off-by line.

I'll try to add a test that exercises this.

@tylerjw tylerjw force-pushed the memcpy-param-overlap branch 2 times, most recently from 79acfab to 1df981d Compare November 3, 2023 15:59
@tylerjw
Copy link
Contributor Author

tylerjw commented Nov 3, 2023

I added a test.

@tylerjw
Add test for memcpy to memmove
0bc8bdb 
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@tylerjw tylerjw force-pushed the memcpy-param-overlap branch from 1df981d to 0bc8bdb Compare November 3, 2023 16:01
@sjahr sjahr mentioned this pull request Nov 3, 2023
Merged
@tylerjw tylerjw mentioned this pull request Nov 3, 2023
Open
clalancette
clalancette requested changes Nov 3, 2023
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.

I tested this locally with enabling asan, and it does show the problem. So the test seems good. I left one thing inline that I think we should add a comment to; once that is added I'll run full CI on this.

@tylerjw
Comment about overlapping areas being UB
626b11a 
Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
@clalancette
Copy link
Contributor

CI:

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

fujitatomoya
fujitatomoya approved these changes Nov 3, 2023
View reviewed changes
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 👍

@clalancette clalancette merged commit 5b99aad into ros2:rolling Nov 4, 2023
@tylerjw tylerjw deleted the memcpy-param-overlap branch November 6, 2023 16:23
tylerjw added a commit to tylerjw/rcutils that referenced this pull request Nov 6, 2023
@tylerjw
memmove for overlaping memory (ros2#434)
1f6846c 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
tylerjw added a commit to tylerjw/rcutils that referenced this pull request Nov 6, 2023
@tylerjw
memmove for overlaping memory (ros2#434)
2d17290 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
Signed-off-by: Tyler Weaver <tyler@picknik.ai>
@clalancette
Copy link
Contributor

@Mergifyio backport iron humble

@mergify Mergify
Copy link

mergify bot commented Nov 7, 2023 •
edited
Loading

backport iron humble

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Nov 7, 2023
@tylerjw @mergify
memmove for overlaping memory (#434)
8554d82 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
(cherry picked from commit 5b99aad)
@mergify mergify bot mentioned this pull request Nov 7, 2023
Merged
mergify bot pushed a commit that referenced this pull request Nov 7, 2023
@tylerjw @mergify
memmove for overlaping memory (#434)
00e76ee 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
(cherry picked from commit 5b99aad)
@mergify mergify bot mentioned this pull request Nov 7, 2023
Merged
clalancette pushed a commit that referenced this pull request Nov 8, 2023
@mergify @tylerjw
memmove for overlaping memory (#434) (#437)
bb00bc0 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
(cherry picked from commit 5b99aad)

Co-authored-by: Tyler Weaver <maybe@tylerjw.dev>
clalancette pushed a commit that referenced this pull request Nov 8, 2023
@mergify @tylerjw
memmove for overlaping memory (#434) (#436)
f37f4e6 
* memmove for overlaping memory

* Add test for memcpy to memmove

Signed-off-by: Tyler Weaver <maybe@tylerjw.dev>
(cherry picked from commit 5b99aad)

Co-authored-by: Tyler Weaver <maybe@tylerjw.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@ahcorde ahcorde Awaiting requested review from ahcorde

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UB: memcpy with overlapping memory in rcutils_array_list_remove when destroying child logger during tear down
3 participants
@tylerjw @clalancette @fujitatomoya