From owner-svn-src-all@freebsd.org Fri Aug 25 22:14:13 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6F018DE29B7; Fri, 25 Aug 2017 22:14:13 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id D7C6F72AB5; Fri, 25 Aug 2017 22:14:12 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 605A71A5DC7; Sat, 26 Aug 2017 07:54:09 +1000 (AEST) Date: Sat, 26 Aug 2017 07:54:08 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Alan Somers cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r322893 - head/bin/dd In-Reply-To: <201708251531.v7PFVtoZ038242@repo.freebsd.org> Message-ID: <20170826061551.A976@besplex.bde.org> References: <201708251531.v7PFVtoZ038242@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=hvvzoT6mHlCTYeWrSbEA:9 a=xkd4CAELegw4TTWS:21 a=nCJHprI3UMGEy-aH:21 a=gEigG_IEjzaiSYIh:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Aug 2017 22:14:13 -0000 On Fri, 25 Aug 2017, Alan Somers wrote: > Log: > dd(1): Incorrect casting of arguments It is indeed now incorrect. > dd(1) casts many of its numeric arguments from uintmax_t to intmax_t and > back again to detect whether or not the original arguments were negative. > This is not correct, and causes problems with boundary cases, for example > when count is SSIZE_MAX-1. The casts were correct. Did someone break the range checks? I wrote most of the cast, but would rarely write range checks using unportable casts. SSIZE_MAX-1 isn't a boundary case. The boundary is at SSIZE_MAX. %zd might work for it not. But %zd is wrong for SSIZE_MAX-1, since the expression might expand the type. > Modified: head/bin/dd/args.c > ============================================================================== > --- head/bin/dd/args.c Fri Aug 25 14:42:11 2017 (r322892) > +++ head/bin/dd/args.c Fri Aug 25 15:31:55 2017 (r322893) > @@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$"); > > #include > > +#include Unrelated change. > #include > #include > #include > @@ -184,7 +185,7 @@ f_bs(char *arg) > > res = get_num(arg); > if (res < 1 || res > SSIZE_MAX) > - errx(1, "bs must be between 1 and %jd", (intmax_t)SSIZE_MAX); > + errx(1, "bs must be between 1 and %zd", SSIZE_MAX); > in.dbsz = out.dbsz = (size_t)res; > } > This is incorrect. The old version used the portable method of casting a signed value of unknown type to intmax_t before %zd existed. %zd still isn't useable in either theory or practice for printing ssize_t's. Theory: C99 defines %zd as being for the signed type "corresponding" to size_t. Who knows what "corresponding" is except for pure 2's complement with no padding bits? POSIX only defines ssize_t circularly as being "any signed integer type capable of representing the range -1 through SSIZE_MAX" where SSIZE_MAX is the maximum of ssize_t". It has a few restrictions that make this not completely circular, but I couldn't find anywhere where it states the usual restriction on limits -- that they have the type of the default promotion of the type that they limit. Practice: As reported in other replies, some arches define SSIZE_MAX in a way that makes its type not the default promotion of ssize_t. Most likely SSIZE_MAX is bogusly long and ssize_t is int, or vice versa, on an arch where int and long have the same representation. Printf format checking reports this type mismatch because it would be fatal on other arches. > @@ -195,22 +196,22 @@ f_cbs(char *arg) > > res = get_num(arg); > if (res < 1 || res > SSIZE_MAX) > - errx(1, "cbs must be between 1 and %jd", (intmax_t)SSIZE_MAX); > + errx(1, "cbs must be between 1 and %zd", SSIZE_MAX); > cbsz = (size_t)res; > } Similarly. > > static void > f_count(char *arg) > { > - intmax_t res; > + uintmax_t res; This breaks the not so careful checking for negative args. dd wants to allow negative args for seek offsets and not much else. It has a badly written get_off_t() to handle the offsets, but old code like this still used the old function get_num() that returns an unsigned type. This is a feature since get_off_t() is so badly written that it is better to convert get_num() almost as done here. My version does this internally: X /* X * Convert an expression of the above forms to an off_t. This version does X * not handle negative numbers perfectly. It assumes 2's complement and X * maybe nonnegative multipliers. I hope perfect handling is not necessary X * for dd. X */ X static off_t X get_off_t(const char *val) X { X u_quad_t num; X X num = get_num(val); X if (num != (u_quad_t)(off_t)num) X errx(1, "%s: offset too large", oper); X return ((off_t)num); X } This still uses the bogus type u_quad_t since that is what get_num() returns in the old version of dd that this patch is for. There should be no restriction except UINTMAX_MAX on counts. This is fixed in my version: X static void X f_count(char *arg) X { X X cpy_cnt = get_num(arg); X #if 0 X if (cpy_cnt == 0) X terminate(0); X #endif X } get_num() already did correct range checking. A count of 0 should mean infinity. Code elsewhere needs be careful to go to infinity for count 0 and to not multiply a large count by a block size. In practice, counts of nearly UINTMAX_MAX are unreachable. > > - res = (intmax_t)get_num(arg); > - if (res < 0) > - errx(1, "count cannot be negative"); > + res = get_num(arg); > + if (res == UINTMAX_MAX) > + errc(1, ERANGE, "%s", oper); This is nonsense error checking. get_num() already did correct checking and exited on range errors. A value of UINTMAX_MAX is not necessarily an error. After calling strtoumax(), it might be either a range error or at the end of the range (including the negative end). Since get_num() returned, it was not a range error. The check here does extra work to break the case where it is in-range. > if (res == 0) > - cpy_cnt = (uintmax_t)-1; > + cpy_cnt = UINTMAX_MAX; > else > - cpy_cnt = (uintmax_t)res; > + cpy_cnt = res; > } I count of 0 always meant infinity. It was converted to the huge value (uintmax_t)-1 which is an alternative spelling of UINTMAX_MAX. My version makes changes elswhere so that 0 works as itself and the user-specified UINTMAX_MAX is not corrupted. > > static void > @@ -219,7 +220,7 @@ f_files(char *arg) > > files_cnt = get_num(arg); > if (files_cnt < 1) > - errx(1, "files must be between 1 and %jd", (uintmax_t)-1); > + errx(1, "files must be between 1 and %zu", SIZE_MAX); > } %jd here was wrong. The value has type uintmax_t. It was printed as -1. Now a different wrong value is printed. The maximum returned by get_num() is still UINTMAX_MAX. This differs from SIZE_MAX on 64-bit arches. My version removes the range check and allows a count of 0. > > static void > @@ -240,8 +241,8 @@ f_ibs(char *arg) > if (!(ddflags & C_BS)) { > res = get_num(arg); > if (res < 1 || res > SSIZE_MAX) Range checks like this are of course correct. Some of the ones with bogus casts might be for portability to systems without OFF_MAX and/or UOFF_MAX. > - errx(1, "ibs must be between 1 and %jd", > - (intmax_t)SSIZE_MAX); > + errx(1, "ibs must be between 1 and %zd", > + SSIZE_MAX); > in.dbsz = (size_t)res; > } > } The change is unportable, as above > @@ -261,8 +262,8 @@ f_obs(char *arg) > if (!(ddflags & C_BS)) { > res = get_num(arg); > if (res < 1 || res > SSIZE_MAX) > - errx(1, "obs must be between 1 and %jd", > - (intmax_t)SSIZE_MAX); > + errx(1, "obs must be between 1 and %zd", > + SSIZE_MAX); > out.dbsz = (size_t)res; > } > } Unportable. > > Modified: head/bin/dd/conv.c > ============================================================================== > --- head/bin/dd/conv.c Fri Aug 25 14:42:11 2017 (r322892) > +++ head/bin/dd/conv.c Fri Aug 25 15:31:55 2017 (r322893) > @@ -133,7 +133,7 @@ block(void) > */ > ch = 0; > for (inp = in.dbp - in.dbcnt, outp = out.dbp; in.dbcnt;) { > - maxlen = MIN(cbsz, in.dbcnt); > + maxlen = MIN(cbsz, (size_t)in.dbcnt); > if ((t = ctab) != NULL) > for (cnt = 0; cnt < maxlen && (ch = *inp++) != '\n'; > ++cnt) Bogus cast to compensate for dbcnt being changed to ssize_t. ssize_t is a necessary evil holding the return value of read() and write() (except the return type should be plain int as in V7 and ssize_t shouldn't exist) (but the main design error was changing the arg type of read() and write() from int to unsigned and then to size_t. The different types give a type morass and args larger than than the signed max are unportable at best). Buffer sizes should have size <= SSIZE_MAX because the behaviour of read() and write() is implementation-defined and hard to use betteen SSIZE_MAX+(size_t)1 and SIZE_MAX. But don't use ssize_t for anything else. Not even for buffer sizes that it can hold. You also changed dbsz to ssize_t. That is the wrong type to pass to read() and write(). It gets converted implicitly by the prototype. The above cast does the same conversion to hide the bug that the types don't match. Both of the implicit conversions might be errors, but compilers don't warn enough for ones in prototypes because they are too common. > @@ -146,7 +146,7 @@ block(void) > * Check for short record without a newline. Reassemble the > * input block. > */ > - if (ch != '\n' && in.dbcnt < cbsz) { > + if (ch != '\n' && (size_t)in.dbcnt < cbsz) { > (void)memmove(in.db, in.dbp - in.dbcnt, in.dbcnt); > break; > } Lots more type poisoning. You didn't spread it to the memmove() call. The second and third args now have type ssize_t instead of size_t. This is hidden by thr prototype. Signed poisoning of cbsz would break the warning about the type mismatch in the comparison. I actually don't like unsigned types, but dd has always used them a lot and needs them to reach 4G on 32-bit systems. All unsigned works almost as well as all signed. > @@ -228,7 +228,7 @@ unblock(void) > * translation has to already be done or we might not recognize the > * spaces. > */ > - for (inp = in.db; in.dbcnt >= cbsz; inp += cbsz, in.dbcnt -= cbsz) { > + for (inp = in.db; (size_t)in.dbcnt >= cbsz; inp += cbsz, in.dbcnt -= cbsz) { > for (t = inp + cbsz - 1; t >= inp && *t == ' '; --t) > ; > if (t >= inp) { This cast also breaks the formatting. > > Modified: head/bin/dd/dd.c > ============================================================================== > --- head/bin/dd/dd.c Fri Aug 25 14:42:11 2017 (r322892) > +++ head/bin/dd/dd.c Fri Aug 25 15:31:55 2017 (r322893) > @@ -204,10 +204,10 @@ setup(void) > * record oriented I/O, only need a single buffer. > */ > if (!(ddflags & (C_BLOCK | C_UNBLOCK))) { > - if ((in.db = malloc(out.dbsz + in.dbsz - 1)) == NULL) > + if ((in.db = malloc((size_t)out.dbsz + in.dbsz - 1)) == NULL) > err(1, "input buffer"); > out.db = in.db; > - } else if ((in.db = malloc(MAX(in.dbsz, cbsz) + cbsz)) == NULL || > + } else if ((in.db = malloc(MAX((size_t)in.dbsz, cbsz) + cbsz)) == NULL || > (out.db = malloc(out.dbsz + cbsz)) == NULL) > err(1, "output buffer"); > Signed poisoning. > @@ -405,7 +405,7 @@ dd_in(void) > ++st.in_full; > > /* Handle full input blocks. */ > - } else if ((size_t)n == in.dbsz) { > + } else if ((size_t)n == (size_t)in.dbsz) { > in.dbcnt += in.dbrcnt = n; > ++st.in_full; > Old bogus cast. n already has type size_t. New signed poisoning. > @@ -562,7 +562,7 @@ dd_out(int force) > outp += nw; > st.bytes += nw; > > - if ((size_t)nw == n && n == out.dbsz) > + if ((size_t)nw == n && n == (size_t)out.dbsz) > ++st.out_full; > else > ++st.out_part; > Lots more signed poisoning. nw holds the value returned by write(), so it needs to be signed (ssize_t). This already poisoned most uses of it. > Modified: head/bin/dd/dd.h > ============================================================================== > --- head/bin/dd/dd.h Fri Aug 25 14:42:11 2017 (r322892) > +++ head/bin/dd/dd.h Fri Aug 25 15:31:55 2017 (r322893) > @@ -38,10 +38,9 @@ > typedef struct { > u_char *db; /* buffer address */ > u_char *dbp; /* current buffer I/O address */ > - /* XXX ssize_t? */ Comment written before someone understood the full brokenness of ssize_t. > - size_t dbcnt; /* current buffer byte count */ Counts should be signed (buffer counts for passing to read()/write() or just adding up). > - size_t dbrcnt; /* last read byte count */ The return value might need to be signed, but it should be converted to unsigned as soon as possible (after checking for errors) to minimise signed poisoning. > - size_t dbsz; /* block size */ Block sizes should be signed. > + ssize_t dbcnt; /* current buffer byte count */ > + ssize_t dbrcnt; /* last read byte count */ > + ssize_t dbsz; /* block size */ > > #define ISCHR 0x01 /* character device (warn on short) */ > #define ISPIPE 0x02 /* pipe-like (see position.c) */ > > Modified: head/bin/dd/position.c > ============================================================================== > --- head/bin/dd/position.c Fri Aug 25 14:42:11 2017 (r322892) > +++ head/bin/dd/position.c Fri Aug 25 15:31:55 2017 (r322893) > @@ -207,7 +207,7 @@ pos_out(void) > n = write(out.fd, out.db, out.dbsz); > if (n == -1) > err(1, "%s", out.name); > - if ((size_t)n != out.dbsz) > + if (n != out.dbsz) > errx(1, "%s: write failure", out.name); > } > break; This moves the ugly ugly conversions to an implicit one in the function call. The error handling is still broken. The non-error of a short write is not handled and this is misreported as a write failure (though correctly using errx()). The signed poisoning makes the critical old and new bugs hard to see. Bruce