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
> 



Reply via email to