Bug 258701
| Summary: | tests: lib/libutil/pidfile_test flaky due to uninitialized array | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | sigsys | ||||
| Component: | tests | Assignee: | Konstantin Belousov <kib> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Some People | CC: | kib, markj | ||||
| Priority: | --- | Keywords: | easy, needs-qa | ||||
| Version: | CURRENT | Flags: | koobs:
maintainer-feedback?
(markj) koobs: mfc-stable13? koobs: mfc-stable12? |
||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Wouldn't it be better to guarantee null-termination of the read string?
diff --git a/lib/libutil/tests/pidfile_test.c b/lib/libutil/tests/pidfile_test.c
index 9e516c35273c..9bfa6754ce54 100644
--- a/lib/libutil/tests/pidfile_test.c
+++ b/lib/libutil/tests/pidfile_test.c
@@ -286,7 +286,8 @@ test_pidfile_relative(void)
fd = open(path, O_RDONLY);
if (fd < 0)
return (strerror(errno));
- if (read(fd, pid, sizeof(pid)) < 0)
+ memset(pid, 0, sizeof(pid));
+ if (read(fd, pid, sizeof(pid) - 1) < 0)
return (strerror(errno));
if (atoi(pid) != getpid())
return ("pid mismatch");
(In reply to Konstantin Belousov from comment #1) Yeah... that's better. Pids can't be this large but if there's a bug anything could be written to that file. And the "" initialization zeroes out the whole array like that memset() but it hides it up there in the middle of declarations. A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=364790beafec707ca3e334683e4030684d829be2 commit 364790beafec707ca3e334683e4030684d829be2 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2021-09-24 03:12:20 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2021-09-24 16:52:41 +0000 pidfile test: guarantee nul termination of the read pid string PR: 258701 Based on the submission by: sigsys@gmail.com MFC after: 1 week lib/libutil/tests/pidfile_test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=288a4784012ca3f2b2c2cb46e67d9c65cd55b5e0 commit 288a4784012ca3f2b2c2cb46e67d9c65cd55b5e0 Author: Konstantin Belousov <kib@FreeBSD.org> AuthorDate: 2021-09-24 03:12:20 +0000 Commit: Konstantin Belousov <kib@FreeBSD.org> CommitDate: 2021-10-01 00:32:22 +0000 pidfile test: guarantee nul termination of the read pid string PR: 258701 (cherry picked from commit 364790beafec707ca3e334683e4030684d829be2) lib/libutil/tests/pidfile_test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) |
Created attachment 228144 [details] patch The char array is used as a string after read(2)'ing directly into it without terminating it. The following atoi() seems pretty tolerant of garbage bytes but it can also get the wrong number or keep scanning past the array and crash if you're unlucky. It was happening a lot for me for some reason.