Skip to content

Conversation

@szsam
Copy link
Contributor

@szsam szsam commented Nov 19, 2025

sizeof(pointer) only gives the pointer size, not the buffer size, so use explicit 10/20 lengths in tests.cpp and update OverflowBuffer.expected to accept the resulting memcpy diagnostics.

sizeof(pointer) only gives the pointer size, not the buffer
size, so use explicit 10/20 lengths in tests.cpp and update
OverflowBuffer.expected to accept the resulting memcpy diagnostics.

Signed-off-by: Mingjie Shen <shen497@purdue.edu>
@szsam szsam requested a review from a team as a code owner November 19, 2025 22:10
@github-actions github-actions bot added the C++ label Nov 19, 2025
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Thanks for noticing this. We would like to be a bit more explicit, see below. Otherwise this looks fine.

Comment on lines 33 to 36
memcpy(bigbuffer, smallbuffer, 10); // GOOD
memcpy(bigbuffer, smallbuffer, 20); // BAD: over-read
memcpy(smallbuffer, bigbuffer, 10); // GOOD
memcpy(smallbuffer, bigbuffer, 20); // BAD: over-write
Copy link
Contributor

Choose a reason for hiding this comment

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

We would prefer the following here:

Suggested change
memcpy(bigbuffer, smallbuffer, 10); // GOOD
memcpy(bigbuffer, smallbuffer, 20); // BAD: over-read
memcpy(smallbuffer, bigbuffer, 10); // GOOD
memcpy(smallbuffer, bigbuffer, 20); // BAD: over-write
memcpy(bigbuffer, smallbuffer, sizeof(char) * 10); // GOOD
memcpy(bigbuffer, smallbuffer, sizeof(char) * 20); // BAD: over-read
memcpy(smallbuffer, bigbuffer, sizeof(char) * 10); // GOOD
memcpy(smallbuffer, bigbuffer, sizeof(char) * 20); // BAD: over-write

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

@geoffw0 geoffw0 merged commit 7db06ca into github:main Nov 27, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants