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 <[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 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 <[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);
> }
> --
> 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);
> - }
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
>
