Date: Mon, 18 Aug 2014 12:00:48 -0700 From: William Orr <will@worrbase.com> To: Alan Somers <asomers@freebsd.org>, FreeBSD CURRENT <freebsd-current@freebsd.org> Subject: Re: Inconsistent behavior with dd(1) Message-ID: <53F24D60.4080107@worrbase.com> In-Reply-To: <20140816173439.GZ83475@funkthat.com> References: <3C86F281-D618-4B93-BBA3-2DA33AC407EC@worrbase.com> <CAOtMX2ikYhyCS0WW178Pn_Ug2pzYQWrNfAvWkJt=f%2BwEdHY3dg@mail.gmail.com> <20140816173439.GZ83475@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8p3vhNA8fQQDtgrGDuLkFO9Fp8BHBDDdn Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Reply inline. On 08/16/2014 10:34 AM, John-Mark Gurney wrote: > Alan Somers wrote this message on Fri, Aug 15, 2014 at 10:42 -0600: >> On Thu, Aug 14, 2014 at 11:55 PM, William Orr <will@worrbase.com> wrot= e: >>> Hey, >>> >>> I found some inconsistent behavior with dd(1) when it comes to specif= ying arguments in -CURRENT. >>> >>> [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D1= 8446744073709551616 >>> dd: count: Result too large >>> [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D1= 8446744073709551617 >>> dd: count: Result too large >>> [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D1= 8446744073709551615 >>> dd: count cannot be negative >>> [ worr on terra ] ( ~ ) % dd if=3D/dev/zero of=3D/dev/null count=3D-= 18446744073709551615 >>> 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 >>> >>> ??? >>> >>> Any chance someone has the time and could take a look? https://bugs.f= reebsd.org/bugzilla/show_bug.cgi?id=3D191263 >>> >>> Thanks, >>> William Orr >>> >>> ??? >> >> >> 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???s 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)SSI= ZE_MAX); >>> - 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. >=20 > The type of dbsz is size_t, so really: >=20 >>> + errx(1, "bs must be between 1 and %ju", (uintmax_t)-1= ); >=20 > This should be SIZE_MAX, except there isn't a define for this? So mayb= e > the code really should be: > (uintmax_t)(size_t)-1 >=20 > to get the correct value for SIZE_MAX... >=20 > Otherwise on systems that uintmax_t is >32bits and size_t is 32bits, > the error message will be wrong... Yes, this should probably be SIZE_MAX rather than that cast. Same with the others >=20 >>> } >>> >>> 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)SS= IZE_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= =2E >=20 > What do you mean by this? cbsz is size_t which is unsigned... I believe he's referring to this use of cbsz/in.dbsz/out.dbsz: https://svnweb.freebsd.org/base/head/bin/dd/dd.c?revision=3D265698&view=3D= markup#l171 Really, this is more wrong since there is math inside of a malloc(3) call without any overflow handling. By virtue of making this max out at a ssize_t, it becomes more unlikely that you'll have overflow. This math should probably be done ahead of time with proper overflow handling. I'll include that in my next patch, if there's no objection. I don't see any other reason why in.dbsz, out.dbsz or cbsz should be signed, but it's very possible that I didn't look hard enough. > Again, the cast above is wrong... Maybe we should add a SIZE_MAX > define so we don't have to see the double cast... >=20 >>> 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) >>> { >>> - >=20 > Don't eliminate these blank lines.. they are intentional per style(9): > /* Insert an empty line if the function has no local varia= bles. */ >=20 >>> 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", (uin= tmax_t)-1); >> >> Again, you eliminated the check for SSIZE_MAX, but dbsz must be signed= =2E >=20 > If dbsz must be signed, we should change it's definition to ssize_t > instead of size_t... Can you point to the line that says this? >=20 > In investigating this, it looks like we may have a bug in ftruncate in > that out.offset * out.dbsz may overflow and return incorrect results...= > We should probably check that the output (cast to off_t) is greater tha= n > both the inputs before calling ftruncate... This is safe as both are > unsigned... Yeah, there probably ought to be integer overflow handling here as well. >=20 >>> } >>> } >>> >>> @@ -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", (uin= tmax_t)-1); >>> } >>> } >> >> Again, you eliminated the check for SSIZE_MAX, but dbsz must be signed= =2E >> >>> >>> @@ -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. Funnily enough this part of the diff was wrong. I didn't account for spaces, so I'll add that in my upcoming diff. >> >>> errno =3D 0; >>> num =3D strtouq(val, &expr, 0); >>> if (errno !=3D 0) /* Overflow or unde= rflow. */ >>> err(1, "%s", oper); >>> - >>> + >>> if (expr =3D=3D val) /* No valid digit= s. */ >>> 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 spec= ified */ >>> - 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. Nope. It didn't do what I wanted. I'll submit an updated diff with this fixed as well as the other things I mentioned, provided there's no objection to my direction. >>> case 0: >>> break; >>> default: >>> >>> >=20 --8p3vhNA8fQQDtgrGDuLkFO9Fp8BHBDDdn Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.14 (GNU/Linux) iQIcBAEBAgAGBQJT8k1lAAoJEOWIWopNkblkBkEP/1o+GE8yL5x/yiFUlx/drGvy p/rbLpvWMALru93/WIbUcLpktT1shOxbT/+uCwFImmrq+AU2nohiHM9zobRDOZgE Iqj6KpJKqhVRpGuG7iwO24Lnv/SKyxYr8Tje/SqLrEwUek49mNsRHcyNvp8M9rii 6tLZV8yYwEs9FnvGH2WH+hiWUFLHt6Ato4l2L50cWbrJrEBJrDuIXqajOFdRlofb O2EQms8XaXnl5toSKVYQOgdy4ORkOXg1Tdp9my6Oph0t8W+cIaIh8XRrEA6UMMos P1CrRib2NQT10Y7ybrWFOff/jz+5FZ0C0PxQXV0gqewA3l9Wp2o4J/5nuKiQSY11 LVq8jgzxtQHSlDKyvXwzOHfTl78Gr/UhE7SJwBpvCgoBgptYZG8P7hQhIYhlXU5W DtlRSrnv7n69MmTWxeB6dC1MmVo3E0eb5J0YvEVjkQKp7E71LAwDPd/hz3VBSQDt 2KU6zSuooSydzrvXcF0qT0aGp4ttYYXly66O2qPQgT7umLlTjpzLU8Rcn1uwbpDB WRTR6anc3urknBu3EuDnOa10eiowDhtlC8x8bkY8MSzbd8EuVFeIjJSD61qWVtKM 5u8DoybOy8M3bD9wcApYILdarwmAlVsELgjMf8r5459GabndAL7QWdRbLLPzLcoj 2iusnBvxbecJTz4oLon4 =CtY/ -----END PGP SIGNATURE----- --8p3vhNA8fQQDtgrGDuLkFO9Fp8BHBDDdn--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?53F24D60.4080107>