Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 25 Aug 2017 10:17:58 -0700
From:      Conrad Meyer <cem@freebsd.org>
To:        Alan Somers <asomers@freebsd.org>
Cc:        src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org,  svn-src-head@freebsd.org
Subject:   Re: svn commit: r322893 - head/bin/dd
Message-ID:  <CAG6CVpU5rsrwU4EEkF3i5gvzSZRX3Q=K%2B8M-LMwk2cGP8hMSxw@mail.gmail.com>
In-Reply-To: <201708251531.v7PFVtoZ038242@repo.freebsd.org>
References:  <201708251531.v7PFVtoZ038242@repo.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
This change seems to break buildworld on MIPS:

/home/cem/head.svn/bin/dd/args.c: In function 'f_bs':
/home/cem/head.svn/bin/dd/args.c:188: warning: format '%zd' expects
type 'signed size_t', but argument 3 has type 'long int'
/home/cem/head.svn/bin/dd/args.c: In function 'f_cbs':
/home/cem/head.svn/bin/dd/args.c:199: warning: format '%zd' expects
type 'signed size_t', but argument 3 has type 'long int'
/home/cem/head.svn/bin/dd/args.c: In function 'f_ibs':
/home/cem/head.svn/bin/dd/args.c:245: warning: format '%zd' expects
type 'signed size_t', but argument 3 has type 'long int'
/home/cem/head.svn/bin/dd/args.c: In function 'f_obs':
/home/cem/head.svn/bin/dd/args.c:266: warning: format '%zd' expects
type 'signed size_t', but argument 3 has type 'long int'

(Yes, it's odd that the SSIZE_MAX constant has 'long' type.)

Best,
Conrad

On Fri, Aug 25, 2017 at 8:31 AM, Alan Somers <asomers@freebsd.org> wrote:
> Author: asomers
> Date: Fri Aug 25 15:31:55 2017
> New Revision: 322893
> URL: https://svnweb.freebsd.org/changeset/base/322893
>
> Log:
>   dd(1): Incorrect casting of arguments
>
>   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.
>
>   PR:           191263
>   Submitted by: will@worrbase.com
>   Reviewed by:  pi, asomers
>   MFC after:    3 weeks
>
> Modified:
>   head/bin/dd/args.c
>   head/bin/dd/conv.c
>   head/bin/dd/dd.c
>   head/bin/dd/dd.h
>   head/bin/dd/position.c
>
> 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 <sys/types.h>
>
> +#include <ctype.h>
>  #include <err.h>
>  #include <errno.h>
>  #include <inttypes.h>
> @@ -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;
>  }
>
> @@ -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;
>  }
>
>  static void
>  f_count(char *arg)
>  {
> -       intmax_t res;
> +       uintmax_t res;
>
> -       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);
>         if (res == 0)
> -               cpy_cnt = (uintmax_t)-1;
> +               cpy_cnt = UINTMAX_MAX;
>         else
> -               cpy_cnt = (uintmax_t)res;
> +               cpy_cnt = res;
>  }
>
>  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);
>  }
>
>  static void
> @@ -240,8 +241,8 @@ f_ibs(char *arg)
>         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);
> +                       errx(1, "ibs must be between 1 and %zd",
> +                           SSIZE_MAX);
>                 in.dbsz = (size_t)res;
>         }
>  }
> @@ -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;
>         }
>  }
>
> 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)
> @@ -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;
>                 }
> @@ -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) {
>
> 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");
>
> @@ -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;
>
> @@ -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;
>
> 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? */
> -       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) */
>
> 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;
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpU5rsrwU4EEkF3i5gvzSZRX3Q=K%2B8M-LMwk2cGP8hMSxw>