Re: a few fixes for cat bugs
On Sat, Jul 02, 2022 at 08:38:53AM +0100, Leah Rowe wrote: > > Hi Otto, > > > Your fixes are not ok. See comment inline. > > The other person (Theo) who responded, raised the same concerns as you. > Sorry for wasting your time. I've reverted the patches myself, locally, > knowing now that I made a few false assumptions. > > I actually overlooked that the buf variable was static. I'm now of the > opinion that OpenBSD's cat implementation is already perfect, and not > in need of new changes. It never hurts going back to code and reread it. To improve you code reviewing skills, I suggest you keep doing it, but be critical of your findings. You'll get better with more experience. Often code reading skills are not valued high enough as opposed to code writing skills, while the former is often more important than the latter, as anybody doing maintenance of a large body of code will tell you. -Otto
Re: a few fixes for cat bugs
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
> 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
> 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
> 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, ) == -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, ) == -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
>
Re: a few fixes for cat bugs
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
> 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
> 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
> 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, ) == -1)
> - err(1, "stdout");
> - bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> - if ((buf = malloc(bsize)) == NULL)
> - err(1, NULL);
> - }
> + if (fstat(wfd, ) == -1)
> + err(1, "stdout");
> + bsize = MAXIMUM(sbuf.st_blksize, BUFSIZ);
> + if ((buf = malloc(bsize)) == NULL)
> +
