Bug 258701 - tests: lib/libutil/pidfile_test flaky due to uninitialized array
Summary: tests: lib/libutil/pidfile_test flaky due to uninitialized array
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: tests (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-testing (Nobody)
URL:
Keywords: easy, needs-qa
Depends on:
Blocks:
 
Reported: 2021-09-24 02:24 UTC by sigsys
Modified: 2021-10-01 00:34 UTC (History)
2 users (show)

See Also:
koobs: maintainer-feedback? (markj)
koobs: mfc-stable13?
koobs: mfc-stable12?


Attachments
patch (439 bytes, patch)
2021-09-24 02:24 UTC, sigsys
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description sigsys 2021-09-24 02:24:46 UTC
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.
Comment 1 Konstantin Belousov freebsd_committer 2021-09-24 03:11:36 UTC
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");
Comment 2 sigsys 2021-09-24 03:29:04 UTC
(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.
Comment 3 commit-hook freebsd_committer 2021-09-24 16:53:55 UTC
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(-)
Comment 4 commit-hook freebsd_committer 2021-10-01 00:34:07 UTC
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(-)