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

Replace memcpy with std::copy to avoid undefined behaviour when buffers overlap. #37

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

LinZhihao-723
Copy link
Member

@LinZhihao-723 LinZhihao-723 commented Nov 26, 2023

Description

In the current implementation, the code uses memcpy to copy the unconsumed bytes from the end of the buffer to the beginning. In most cases, this is safe because the previous if statement handles the cases where more than half of bytes in the current buffer are unconsumed. However, the corner case happens when the number of unconsumed bytes is larger than half of the actual buffer size (not the capacity). If this happens, the behavior of the code depends on the underlying memcpy implementation. This PR solves this UB by replacing memcpy with std::copy, so it will be safe even if the src and dst buffers overlap.
Notice that there will be a performance drop with the changes in this PR since std::copy cannot optimize gsl::span. However, this problem will be fix after switching to C++20 with std::span.

Copy link
Member

@davidlion davidlion left a comment

Choose a reason for hiding this comment

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

Lets make this copy use c++.

@LinZhihao-723 LinZhihao-723 changed the title Replace memcpy by memmove to avoid undefined behaviour when the internal buffers overlap. Replace memcpy by std::copy to avoid undefined behaviour when the internal buffers overlap. Nov 27, 2023
@LinZhihao-723 LinZhihao-723 changed the title Replace memcpy by std::copy to avoid undefined behaviour when the internal buffers overlap. Replace memcpy with std::copy to avoid undefined behaviour when internal buffers overlap. Nov 27, 2023
@LinZhihao-723 LinZhihao-723 changed the title Replace memcpy with std::copy to avoid undefined behaviour when internal buffers overlap. Replace memcpy with std::copy to avoid undefined behaviour when buffers overlap. Nov 27, 2023
@LinZhihao-723 LinZhihao-723 merged commit 0a9a622 into y-scope:main Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants