Date: Mon, 28 May 2001 18:40:31 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Mike Heffner <mheffner@cowpie.acm.vt.edu> Cc: freebsd-audit@FreeBSD.ORG Subject: Re: cmp(1) patch Message-ID: <Pine.BSF.4.21.0105281757490.485-100000@besplex.bde.org> In-Reply-To: <20010527232926.A85869@cowpie.acm.vt.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 27 May 2001, Mike Heffner wrote:
> BDECFLAGS cleanup, respect -s flag in one place, return instead of
> exit() at end of main. Please review.
>
> Index: cmp.c
> ===================================================================
> RCS file: /home/ncvs/src/usr.bin/cmp/cmp.c,v
> retrieving revision 1.9
> diff -u -r1.9 cmp.c
> --- cmp.c 2000/07/25 13:01:34 1.9
> +++ cmp.c 2001/05/26 21:57:56
> ...
> @@ -117,9 +118,13 @@
> exit(1);
> }
> if (strcmp(file2 = argv[1], "-") == 0) {
> - if (special)
> - errx(ERR_EXIT,
> + if (special) {
> + if (!sflag)
> + errx(ERR_EXIT,
> "standard input may only be specified once");
> + else
> + exit(1);
> + }
> special = 1;
> fd2 = 0;
> file2 = "stdin";
This change seems to be just a bug. -s is only required (by POSIX) to
suppress output to stdout (in particular, it must suppress the message
about differing files). The above suppresses output to stderr for a
usage error. Output to stderr is still done for other usage errors.
> ...
> @@ -160,14 +165,14 @@
> else {
> if (zflag && sb1.st_size != sb2.st_size) {
> if (!sflag)
> - (void) printf("%s %s differ: size\n",
> + (void)printf("%s %s differ: size\n",
> file1, file2);
> exit(DIFF_EXIT);
> }
> c_regular(fd1, file1, skip1, sb1.st_size,
> fd2, file2, skip2, sb2.st_size);
> }
> - exit(0);
> + return (0);
gcc should understand that exit() doesn't return, and not warn for exit(0).
> Index: regular.c
> ===================================================================
> RCS file: /home/ncvs/src/usr.bin/cmp/regular.c,v
> retrieving revision 1.10
> diff -u -r1.10 regular.c
> --- regular.c 2000/06/20 20:28:40 1.10
> +++ regular.c 2001/05/26 21:57:57
> ...
> @@ -81,8 +81,10 @@
> off2 = ROUNDPAGE(skip2);
>
> length = MIN(len1, len2);
> - if (length > SIZE_T_MAX)
> - return (c_special(fd1, file1, skip1, fd2, file2, skip2));
> + if (length > SIZE_T_MAX) {
> + c_special(fd1, file1, skip1, fd2, file2, skip2);
> + return;
> + }
>
> if ((p1 = (u_char *)mmap(NULL, (size_t)len1 + skip1 % pagesize,
> PROT_READ, MAP_SHARED, fd1, off1)) == (u_char *)MAP_FAILED)
This code has several bugs that should be fixed someday:
- casting len1 (and later len2) to size_t may overflow. We only checked
that MIN(len1, len2) fits in a size_t. mmap()'ing more than `length'
(rounded up a little?) bytes is a bug. The CSRG version was missing
this bug, but apparently had others.
- adding skip1 % pagesize may cause another overflow.
- the mmap() (or the later one) may fail for various reasons. E.g., the
files may have length 3GB each. It is impossible for mmap() to map
two files of length 3GB each on i386's, since there is at most 4GB of
vm to map them in (the mmap() will actually fail much earlier, near
2 * 1.25GB). c_special() should be used if either of the mmap()s fails.
Note that the SIZE_T_MAX check isn't intended to make mmap() work. It
is just intended to prevent overflow when the size arg for mmap is
squeezed into a size_t. But even that is botched here.
Many utilities that call mmap() have similar bugs. One already has a wrong
fix for mmap() failure.
> @@ -101,10 +103,12 @@
> if ((ch = *p1) != *p2) {
> if (xflag) {
> dfound = 1;
> - (void)printf("%08qx %02x %02x\n", byte - 1, ch, *p2);
> + (void)printf("%08llx %02x %02x\n",
> + (long long)byte - 1, ch, *p2);
Using hard "long long" breaks ISO C (C90) support, not to mention K&R
support, and is also wrong for C99. intmax_t should be used for C99,
but we don't have either intma_t or a C99 compiler yet.
There are still printf format errors here. %llx is for unsigned type, but
both the type of `byte' and "long long" are signed types.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0105281757490.485-100000>