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

time-util: fix buffer-over-run #23933

Merged
merged 1 commit into from Jul 8, 2022
Merged

time-util: fix buffer-over-run #23933

merged 1 commit into from Jul 8, 2022

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Jul 7, 2022

Fixes #23928.

src/test/test-time-util.c Outdated Show resolved Hide resolved
@@ -238,6 +238,11 @@ TEST(format_timespan) {
test_format_timespan_accuracy(1);
test_format_timespan_accuracy(USEC_PER_MSEC);
test_format_timespan_accuracy(USEC_PER_SEC);

/* See issue #23928. */
_cleanup_free_ char *buf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just char buf[5]?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know why, but valgrind does not detect the buffer-over-run if buf[5] is used.
ASAN may detect the issue even with buf[5]. But this is just a test code, hence I hope that's no problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not know why, but valgrind does not detect the buffer-over-run if buf[5] is used.

Because valgrind's memcheck doesn't check stack allocations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks.

@poettering poettering added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Jul 8, 2022
@poettering poettering merged commit 9102c62 into systemd:main Jul 8, 2022
@yuwata yuwata deleted the time-util branch July 8, 2022 14:55
@keszybz keszybz removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Buffer overrun in format_timespan
6 participants