From owner-freebsd-current@FreeBSD.ORG Sat Sep 20 20:04:37 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id A183A970 for ; Sat, 20 Sep 2014 20:04:37 +0000 (UTC) Received: from mail-oa0-x232.google.com (mail-oa0-x232.google.com [IPv6:2607:f8b0:4003:c02::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6CFDF387 for ; Sat, 20 Sep 2014 20:04:37 +0000 (UTC) Received: by mail-oa0-f50.google.com with SMTP id jd19so2836298oac.9 for ; Sat, 20 Sep 2014 13:04:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:date:message-id:subject:from:to:cc:content-type; bh=uC9BrBFhjA4e9N/O5QUNOii+Ho7jcBoHadP0aCWTntU=; b=N8NeY7vTsO3W54TpuR+NycWo5A0U502KiIE5l1RxfOgPSMplW8Z5afCRQ5ZFnqQGkg iOhCdJgjs6VnruOfYtfVS0rrw5QEVeJtVQZ1GMEskXFsKHb66Pw9HMqwx+k631aSrKsx QGG0IY4myXl6V+QZ9BwvZiObMf9LK2yEdhV0S3DITPjM2A1C/t6J8dnUUBSYuRSTnVms 6YDNonoqekRGCnLM10Zm+COWHM/cpges9T6eIgk7BbvNccJtk8wI4/OZ0qKthOmLWkKR RbsTeXwWJ9Vu7ZfdsDwanXNBPOqxgRedI9tGbWYngwAv+uCZIhXJJwBZqaOhy+LGyEPA rtUQ== MIME-Version: 1.0 X-Received: by 10.182.153.68 with SMTP id ve4mr17132435obb.60.1411243476678; Sat, 20 Sep 2014 13:04:36 -0700 (PDT) Received: by 10.182.228.51 with HTTP; Sat, 20 Sep 2014 13:04:36 -0700 (PDT) Date: Sat, 20 Sep 2014 22:04:36 +0200 Message-ID: Subject: memo: Re: [PATCH] Fix integer overflow handling in dd(1) From: Oliver Pinter To: dev@hardenedbsd.org Content-Type: text/plain; charset=ISO-8859-1 Cc: freebsd-current@freebsd.org, William Orr X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 20 Sep 2014 20:04:37 -0000 pick On 9/20/14, William Orr wrote: > Hey, > > I've submitted this patch before, and it's gotten comments and fixes, > but still hasn't been merged. Any thoughts? Does it need more work? > > Thanks, > William Orr > > Index: args.c > =================================================================== > --- args.c (revision 270645) > +++ args.c (working copy) > @@ -41,6 +41,7 @@ > > #include > > +#include > #include > #include > #include > @@ -171,8 +172,7 @@ > */ > if (in.offset > OFF_MAX / (ssize_t)in.dbsz || > out.offset > OFF_MAX / (ssize_t)out.dbsz) > - errx(1, "seek offsets cannot be larger than %jd", > - (intmax_t)OFF_MAX); > + errx(1, "seek offsets cannot be larger than %jd", OFF_MAX); > } > > static int > @@ -186,37 +186,28 @@ > static void > f_bs(char *arg) > { > - uintmax_t res; > > - res = get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "bs must be between 1 and %jd", (intmax_t)SSIZE_MAX); > - in.dbsz = out.dbsz = (size_t)res; > + in.dbsz = out.dbsz = get_num(arg); > + if (out.dbsz < 1 || out.dbsz > SSIZE_MAX) > + errx(1, "bs must be between 1 and %jd", SSIZE_MAX); > } > > static void > f_cbs(char *arg) > { > - uintmax_t res; > > - res = get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "cbs must be between 1 and %jd", (intmax_t)SSIZE_MAX); > - cbsz = (size_t)res; > + cbsz = get_num(arg); > + if (cbsz < 1 || cbsz > SSIZE_MAX) > + errx(1, "cbs must be between 1 and %jd", SSIZE_MAX); > } > > static void > f_count(char *arg) > { > - intmax_t res; > > - res = (intmax_t)get_num(arg); > - if (res < 0) > - errx(1, "count cannot be negative"); > - if (res == 0) > - cpy_cnt = (uintmax_t)-1; > - else > - cpy_cnt = (uintmax_t)res; > + cpy_cnt = get_num(arg); > + if (cpy_cnt == 0) > + cpy_cnt = -1; > } > > static void > @@ -225,7 +216,7 @@ > > 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 %ju", SIZE_MAX); > } > > static void > @@ -241,14 +232,11 @@ > static void > f_ibs(char *arg) > { > - uintmax_t res; > > if (!(ddflags & C_BS)) { > - res = get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "ibs must be between 1 and %jd", > - (intmax_t)SSIZE_MAX); > - in.dbsz = (size_t)res; > + in.dbsz = get_num(arg); > + if (in.dbsz < 1 || in.dbsz > SSIZE_MAX) > + errx(1, "ibs must be between 1 and %ju", SSIZE_MAX); > } > } > > @@ -262,14 +250,11 @@ > static void > f_obs(char *arg) > { > - uintmax_t res; > > 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); > - out.dbsz = (size_t)res; > + out.dbsz = get_num(arg); > + if (out.dbsz < 1 || out.dbsz > SSIZE_MAX) > + errx(1, "obs must be between 1 and %jd", SSIZE_MAX); > } > } > > @@ -378,11 +363,17 @@ > uintmax_t num, mult, prevnum; > char *expr; > > + while (isspace(val[0])) > + val++; > + > + if (val[0] == '-') > + errx(1, "%s: cannot be negative", oper); > + > errno = 0; > - num = strtouq(val, &expr, 0); > + num = strtoull(val, &expr, 0); > if (errno != 0) /* Overflow or underflow. */ > err(1, "%s", oper); > - > + > if (expr == val) /* No valid digits. */ > errx(1, "%s: illegal numeric value", oper); > > Index: conv.c > =================================================================== > --- conv.c (revision 270645) > +++ conv.c (working copy) > @@ -133,7 +133,7 @@ > */ > 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) > @@ -146,7 +146,7 @@ > * 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; > } > @@ -228,7 +228,7 @@ > * 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) { > Index: dd.c > =================================================================== > --- dd.c (revision 270645) > +++ dd.c (working copy) > @@ -168,10 +168,10 @@ > * 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"); > > @@ -343,7 +343,7 @@ > ++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; > > @@ -493,7 +493,7 @@ > 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; > Index: dd.h > =================================================================== > --- dd.h (revision 270645) > +++ dd.h (working copy) > @@ -38,10 +38,9 @@ > typedef struct { > u_char *db; /* buffer address */ > u_char *dbp; /* current buffer I/O address */ > - /* XXX ssize_t? */ > - size_t dbcnt; /* current buffer byte count */ > - size_t dbrcnt; /* last read byte count */ > - size_t dbsz; /* block size */ > + 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) */ > @@ -57,13 +56,13 @@ > } IO; > > typedef struct { > - uintmax_t in_full; /* # of full input blocks */ > - uintmax_t in_part; /* # of partial input blocks */ > - uintmax_t out_full; /* # of full output blocks */ > - uintmax_t out_part; /* # of partial output blocks */ > - uintmax_t trunc; /* # of truncated records */ > - uintmax_t swab; /* # of odd-length swab blocks */ > - uintmax_t bytes; /* # of bytes written */ > + size_t in_full; /* # of full input blocks */ > + size_t in_part; /* # of partial input blocks */ > + size_t out_full; /* # of full output blocks */ > + size_t out_part; /* # of partial output blocks */ > + size_t trunc; /* # of truncated records */ > + size_t swab; /* # of odd-length swab blocks */ > + size_t bytes; /* # of bytes written */ > struct timespec start; /* start time of dd */ > } STAT; > > Index: position.c > =================================================================== > --- position.c (revision 270645) > +++ position.c (working copy) > @@ -178,7 +178,7 @@ > 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; > _______________________________________________ > freebsd-current@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to "freebsd-current-unsubscribe@freebsd.org" >