On Sat, Jul 02, 2022 at 03:15:21AM +0100, Leah Rowe wrote: > > Hi, > > I've been playing around with a few OpenBSD userland programs. By that, > I mean I've been studying them extensively. I'm working on creating a > busybox-like userland for Linux with musl libc, and I need quality code > so I decided I'd start ripping code out of OpenBSD for this purpose. > > I spent today obsessing over your cat implementation. As part of my > optimization efforts (I've been removing non-POSIX features, because I > need the code to be as small as possible), I found several minor issues > in cat: > > 1) Unitialized variables that are assumed to be zero, judging by the > logic in the code that uses them.
They are not used uninitialized. > > 2) Memory leak; in practise, most people will run can on a few files > and it won't take more than a few moments, then cat terminates and the > memory is freed Not really a leak. The static buf will not be freed before exit, but that's fine. Your diff introduces a pretty bad bug. > > 3) Unnecessary check in raw_cat on a variable being NULL, when it will > always evaluate true because it's explicitly already initialized to null You are removing an optimization and fixing the bug you introduced in 2. > > I fixed all of these, in the attached patches which I present for your > enjoyment. I've done these on top of your github mirror, on top of > commit 88e033f9985d9aec739c4497f51fb9348247d4db of the main OpenBSD git > repository (I don't know how to use CVS, I've never used it, sorry! I > got into programming in the late 2000s right when git was made lol) > > -- > Leah Rowe, > Company Director > > Minifree Ltd, trading as Ministry of Freedom. > Registered in England, registration No. 9361826 > VAT Registration No. GB202190462 > Minifree Ltd, 19 Hilton Road, Canvey Island > Essex SS8 9QA, United Kingdom > United Kingdom > From 63163120b6881f9d8ea2b03f722cd31e946f5704 Mon Sep 17 00:00:00 2001 > From: Leah Rowe <[email protected]> > Date: Sat, 2 Jul 2022 02:27:51 +0100 > Subject: [PATCH 1/3] cat: fix uninitialized variables > > --- > bin/cat/cat.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/bin/cat/cat.c b/bin/cat/cat.c > index a2eff2e1b30..42960812ead 100644 > --- a/bin/cat/cat.c > +++ b/bin/cat/cat.c > @@ -62,6 +62,8 @@ main(int argc, char *argv[]) > if (pledge("stdio rpath", NULL) == -1) > err(1, "pledge"); > > + rval = bflag = eflag = nflag = sflag = tflag = vflag = 0; These are global variables, so they are initialized to 0. > + > while ((ch = getopt(argc, argv, "benstuv")) != -1) { > switch (ch) { > case 'b': > @@ -210,7 +212,7 @@ void > raw_cat(int rfd, const char *filename) > { > int wfd; > - ssize_t nr, nw, off; > + ssize_t nr, off, nw = 0; for (off = 0; nr; nr -= nw, off += nw) { if ((nw = write(wfd, buf + off, nr)) == -1 || nw == 0) err(1, "stdout"); } nw is only used after the body of the for loop was run once, so it isn't used uninitialized. > static size_t bsize; > static char *buf = NULL; > struct stat sbuf; > -- > 2.25.1 > > From e36d05353d66cb42d33f9b5d5eb3e4dc44175e39 Mon Sep 17 00:00:00 2001 > From: Leah Rowe <[email protected]> > Date: Sat, 2 Jul 2022 02:28:33 +0100 > Subject: [PATCH 2/3] cat: fix memory leak > > --- > bin/cat/cat.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/bin/cat/cat.c b/bin/cat/cat.c > index 42960812ead..dad337d5f68 100644 > --- a/bin/cat/cat.c > +++ b/bin/cat/cat.c > @@ -235,4 +235,5 @@ raw_cat(int rfd, const char *filename) > warn("%s", filename); > rval = 1; > } > + free(buf); buf is a static variable. It is allocated once, namely the first time raw_cat is called. This diff introduces a use-after-free: $ obj/cat cat.c cat.1 >/dev/null cat(35527) in free(): bogus pointer (double free?) 0xa5fc8145000 Abort trap (core dumped) > } > -- > 2.25.1 > > From b4e0ef740724523ed3752c6f5b82741d1acc5230 Mon Sep 17 00:00:00 2001 > From: Leah Rowe <[email protected]> > Date: Sat, 2 Jul 2022 02:29:20 +0100 > Subject: [PATCH 3/3] cat: remove unnecessary check > > --- > bin/cat/cat.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/bin/cat/cat.c b/bin/cat/cat.c > index dad337d5f68..97edcad2101 100644 > --- a/bin/cat/cat.c > +++ b/bin/cat/cat.c > @@ -218,13 +218,12 @@ raw_cat(int rfd, const char *filename) > struct stat sbuf; > > wfd = fileno(stdout); > - if (buf == NULL) { > - if (fstat(wfd, &sbuf) == -1) > - err(1, "stdout"); > - bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ); > - if ((buf = malloc(bsize)) == NULL) > - err(1, NULL); > - } > + if (fstat(wfd, &sbuf) == -1) > + err(1, "stdout"); > + bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ); > + if ((buf = malloc(bsize)) == NULL) > + err(1, NULL); > + While this fixes the use-after-free you introduced in the previous patch, you now allocate every time raw_cat() is called, so this is not an improvement. > while ((nr = read(rfd, buf, bsize)) != -1 && nr != 0) { > for (off = 0; nr; nr -= nw, off += nw) { > if ((nw = write(wfd, buf + off, nr)) == -1 || nw == 0) > -- > 2.25.1 >
