Date: Fri, 15 Aug 2014 10:42:13 -0600 From: Alan Somers <asomers@freebsd.org> To: William Orr <will@worrbase.com> Cc: FreeBSD CURRENT <freebsd-current@freebsd.org> Subject: Re: Inconsistent behavior with dd(1) Message-ID: <CAOtMX2ikYhyCS0WW178Pn_Ug2pzYQWrNfAvWkJt=f%2BwEdHY3dg@mail.gmail.com> In-Reply-To: <3C86F281-D618-4B93-BBA3-2DA33AC407EC@worrbase.com> References: <3C86F281-D618-4B93-BBA3-2DA33AC407EC@worrbase.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Aug 14, 2014 at 11:55 PM, William Orr <will@worrbase.com> wrote: > Hey, > > I found some inconsistent behavior with dd(1) when it comes to specifying= arguments in -CURRENT. > > [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D18446= 744073709551616 > dd: count: Result too large > [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D18446= 744073709551617 > dd: count: Result too large > [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D18446= 744073709551615 > dd: count cannot be negative > [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D-1844= 6744073709551615 > 1+0 records in > 1+0 records out > 512 bytes transferred in 0.000373 secs (1373071 bytes/sec) > [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D-1 > dd: count cannot be negative > > =E2=80=94 > > Any chance someone has the time and could take a look? https://bugs.freeb= sd.org/bugzilla/show_bug.cgi?id=3D191263 > > Thanks, > William Orr > > =E2=80=94 IMHO, this is a bug in strtouq(3), not in dd(1). Why should it parse negative numbers at all, when there is stroq(3) for that purpose? The standard is clear that it must, though. Oddly enough, stroq would probably not accept -18446744073709551615, even though strtouq does. Specific comments on your patch below: > > Here=E2=80=99s the patch: > > Index: bin/dd/args.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bin/dd/args.c (revision 267712) > +++ bin/dd/args.c (working copy) > @@ -186,46 +186,31 @@ > static void > f_bs(char *arg) > { > - uintmax_t res; > - > - res =3D get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "bs must be between 1 and %jd", (intmax_t)SSIZE_M= AX); > - in.dbsz =3D out.dbsz =3D (size_t)res; > + in.dbsz =3D out.dbsz =3D get_num(arg); > + if (in.dbsz < 1 || out.dbsz < 1) Why do you need to check both in and out? Aren't they the same? Also, you eliminated the check for overflowing SSIZE_MAX. That's not ok, because these values get passed to places that expect signed numbers, for example in dd.c:303. > + errx(1, "bs must be between 1 and %ju", (uintmax_t)-1); > } > > static void > f_cbs(char *arg) > { > - uintmax_t res; > - > - res =3D get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "cbs must be between 1 and %jd", (intmax_t)SSIZE_= MAX); > - cbsz =3D (size_t)res; > + cbsz =3D get_num(arg); > + if (cbsz < 1) > + errx(1, "cbs must be between 1 and %ju", (uintmax_t)-1); > } Again, you eliminated the check for SSIZE_MAX, but cbsz must be signed. > > static void > f_count(char *arg) > { > - intmax_t res; > - > - res =3D (intmax_t)get_num(arg); > - if (res < 0) > - errx(1, "count cannot be negative"); > - if (res =3D=3D 0) > - cpy_cnt =3D (uintmax_t)-1; This is a special case. See dd_in(). I think that eliminating this special case will have the unintended effect of breaking count=3D0. > - else > - cpy_cnt =3D (uintmax_t)res; > + cpy_cnt =3D get_num(arg); > } > > static void > f_files(char *arg) > { > - > files_cnt =3D 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", (uintmax_t)-1)= ; Good catch. > } > > static void > @@ -241,14 +226,10 @@ > static void > f_ibs(char *arg) > { > - uintmax_t res; > - > if (!(ddflags & C_BS)) { > - res =3D get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "ibs must be between 1 and %jd", > - (intmax_t)SSIZE_MAX); > - in.dbsz =3D (size_t)res; > + in.dbsz =3D get_num(arg); > + if (in.dbsz < 1) > + errx(1, "ibs must be between 1 and %ju", (uintmax= _t)-1); Again, you eliminated the check for SSIZE_MAX, but dbsz must be signed. > } > } > > @@ -262,14 +243,10 @@ > static void > f_obs(char *arg) > { > - uintmax_t res; > - > if (!(ddflags & C_BS)) { > - res =3D get_num(arg); > - if (res < 1 || res > SSIZE_MAX) > - errx(1, "obs must be between 1 and %jd", > - (intmax_t)SSIZE_MAX); > - out.dbsz =3D (size_t)res; > + out.dbsz =3D get_num(arg); > + if (out.dbsz < 1) > + errx(1, "obs must be between 1 and %ju", (uintmax= _t)-1); > } > } Again, you eliminated the check for SSIZE_MAX, but dbsz must be signed. > > @@ -378,11 +355,14 @@ > uintmax_t num, mult, prevnum; > char *expr; > > + if (val[0] =3D=3D '-') > + errx(1, "%s: cannot be negative", oper); > + In general, I like this part of the diff. Every user of get_num checks for negative values, so why not move the check into get_num itself? But you changed it from a numeric check to a text check, and writing text parsers makes me nervous. I can't see any problems, though. > errno =3D 0; > num =3D strtouq(val, &expr, 0); > if (errno !=3D 0) /* Overflow or underflo= w. */ > err(1, "%s", oper); > - > + > if (expr =3D=3D val) /* No valid digits. *= / > errx(1, "%s: illegal numeric value", oper); > > Index: bin/dd/dd.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- bin/dd/dd.c (revision 267712) > +++ bin/dd/dd.c (working copy) > @@ -284,8 +284,6 @@ > > for (;;) { > switch (cpy_cnt) { > - case -1: /* count=3D0 was specifie= d */ > - return; Again, I don't think this will do what you want it to do. Previously, leaving count unspecified resulted in cpy_cnt being 0, and specifying count=3D0 set cpy_cnt to -1. With your patch, setting count=3D0 will have the same effect as leaving it unspecified. > case 0: > break; > default: > > -Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAOtMX2ikYhyCS0WW178Pn_Ug2pzYQWrNfAvWkJt=f%2BwEdHY3dg>