From owner-freebsd-audit Mon May 28 1:42:20 2001 Delivered-To: freebsd-audit@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id F33F937B424 for ; Mon, 28 May 2001 01:42:14 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id SAA07315; Mon, 28 May 2001 18:42:03 +1000 Date: Mon, 28 May 2001 18:40:31 +1000 (EST) From: Bruce Evans X-Sender: bde@besplex.bde.org To: Mike Heffner Cc: freebsd-audit@FreeBSD.ORG Subject: Re: cmp(1) patch In-Reply-To: <20010527232926.A85869@cowpie.acm.vt.edu> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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