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. > > 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 > > 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 > > 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)
Your fixes are not ok. See comment inline. -Otto > -- > 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 <l...@libreboot.org> > 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 globals, which are initialized to zero by the runtime before main() is called. > 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; nw is initialized by the code below (with the return value of write()) before being used. > 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 <l...@libreboot.org> > 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); > } > -- > 2.25.1 > > From b4e0ef740724523ed3752c6f5b82741d1acc5230 Mon Sep 17 00:00:00 2001 > From: Leah Rowe <l...@libreboot.org> > 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); > - } You don't seem to understand how static vars work. The idea is that that raw cat re-uses the buffer as buf is a static. No need to free it, after process exit the memory will be cleaned up. > + if (fstat(wfd, &sbuf) == -1) > + err(1, "stdout"); > + bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ); > + if ((buf = malloc(bsize)) == NULL) > + err(1, NULL); > + > 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 >