Date: Mon, 28 May 2001 12:02:08 -0400 From: Mike Heffner <mheffner@cowpie.acm.vt.edu> To: Bruce Evans <bde@zeta.org.au> Cc: freebsd-audit@freebsd.org Subject: Re: cmp(1) patch Message-ID: <20010528120208.A87924@cowpie.acm.vt.edu> In-Reply-To: <Pine.BSF.4.21.0105281757490.485-100000@besplex.bde.org>; from bde@zeta.org.au on Mon, May 28, 2001 at 06:40:31PM %2B1000 References: <20010527232926.A85869@cowpie.acm.vt.edu> <Pine.BSF.4.21.0105281757490.485-100000@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, May 28, 2001 at 06:40:31PM +1000, Bruce Evans wrote:
| 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.
However it is also used to suppress output from open and fstat
failing, but I guess in the case of scripts this might be
favorable. I'll reverse this part since it's a usage error though.
|
| > ...
| > @@ -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).
Ok, I just thought return looked better at the end of main =), but I'll
leave exit() in there.
|
| > 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.
Changing the 'if (length > SIZE_T_MAX)' to 'if (MAX(len1, len2) >
SIZE_T_MAX)' (plus possibly the skip lengths) should fix the
overflowing on size_t issue right?
| - 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.
I'll try to address this issue, to call c_special when mmap fails and
fix the overflow checks, when I redo the patch.
|
| 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.
So should it be left with `%qx', or should byte be typecasted unsigned and use
`%llx'?
Thanks for looking at this by the way ;)
--
Mike Heffner <mheffner@vt.edu>
Fredericksburg, VA <mikeh@FreeBSD.org>
http://filebox.vt.edu/users/mheffner
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?20010528120208.A87924>