memo: Re: [PATCH] Fix integer overflow handling in dd(1)
Oliver Pinter
oliver.pntr at gmail.com
Sat Sep 20 20:04:37 UTC 2014
pick
On 9/20/14, William Orr <will at worrbase.com> 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 <sys/types.h>
>
> +#include <ctype.h>
> #include <err.h>
> #include <errno.h>
> #include <inttypes.h>
> @@ -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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe at freebsd.org"
>
More information about the freebsd-current
mailing list