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

Replaced strncpy with memcpy #684

Merged
merged 3 commits into from
Apr 5, 2019
Merged

Replaced strncpy with memcpy #684

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08d39e8
Replaced strncpy with memcpy
ivanpauno Apr 5, 2019
36f1d93
Use rcutils_snprintf in place of std::strncpy
ivanpauno Apr 5, 2019
7005f27
Used memcpy, with bound checking
ivanpauno Apr 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rclcpp/include/rclcpp/intra_process_manager_impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ class IntraProcessManagerImpl : public IntraProcessManagerImplBase
fixed_size_string(const char * str) const
{
FixedSizeString ret;
std::strncpy(ret.data(), str, ret.size());
Copy link
Member

Choose a reason for hiding this comment

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

The previous code already looks wrong. I always copies ret.size() bytes - even if str is shorter much implies copying undefined data.

This should check the strlen(str), ensure that it is <= RMW_TOPIC_MAX_NAME_LENGTH and only copy its actual data.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, std::strncpy pads with zeros after the '\0'. I'm using now rcutils_snprintf, which stops copying in the null terminator and solves the same portability issue.
I'm adding the strcmp_wrapper again, as the lexicographical comparison of std::array doesn't stop in the null terminator. I didn't realize before, and as std::strncpy pads with zeros, it was passing.

Copy link
Member

Choose a reason for hiding this comment

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

Why use the complexity of snprintf if all needed is a 1-to-1 copy?

std::memcpy(ret.data(), str, ret.size());
return ret;
}

Expand Down