memo: Re: [PATCH] Fix integer overflow handling in dd(1)

From: Oliver Pinter <oliver.pntr_at_gmail.com>
Date: Sat, 20 Sep 2014 22:04:36 +0200
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)
> _at__at_ -41,6 +41,7 _at__at_
>
>   #include <sys/types.h>
>
> +#include <ctype.h>
>   #include <err.h>
>   #include <errno.h>
>   #include <inttypes.h>
> _at__at_ -171,8 +172,7 _at__at_
>        */
>       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
> _at__at_ -186,37 +186,28 _at__at_
>   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
> _at__at_ -225,7 +216,7 _at__at_
>
>       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
> _at__at_ -241,14 +232,11 _at__at_
>   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);
>       }
>   }
>
> _at__at_ -262,14 +250,11 _at__at_
>   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);
>       }
>   }
>
> _at__at_ -378,11 +363,17 _at__at_
>       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)
> _at__at_ -133,7 +133,7 _at__at_
>        */
>       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)
> _at__at_ -146,7 +146,7 _at__at_
>            * 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;
>           }
> _at__at_ -228,7 +228,7 _at__at_
>        * 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)
> _at__at_ -168,10 +168,10 _at__at_
>        * 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");
>
> _at__at_ -343,7 +343,7 _at__at_
>               ++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;
>
> _at__at_ -493,7 +493,7 _at__at_
>               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)
> _at__at_ -38,10 +38,9 _at__at_
>   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) */
> _at__at_ -57,13 +56,13 _at__at_
>   } 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)
> _at__at_ -178,7 +178,7 _at__at_
>               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"
>
Received on Sat Sep 20 2014 - 18:04:37 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:52 UTC