From owner-freebsd-current@FreeBSD.ORG Fri Aug 15 16:42:15 2014 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id EA268906 for ; Fri, 15 Aug 2014 16:42:15 +0000 (UTC) Received: from mail-we0-x230.google.com (mail-we0-x230.google.com [IPv6:2a00:1450:400c:c03::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7765C28A9 for ; Fri, 15 Aug 2014 16:42:15 +0000 (UTC) Received: by mail-we0-f176.google.com with SMTP id q58so2560641wes.21 for ; Fri, 15 Aug 2014 09:42:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type:content-transfer-encoding; bh=spU9b5ZWaqXPOwk8e+kg3MGeq2gw7PyklmTU6Cos8OM=; b=zfE2xHoQskkZu321P3Thx2pJyNM3LWv8xjc1e9NUXxzUHg73t6o2U4avrzNcX1RvkQ U35qwIryrM2WaYTkqzOGnz9KgGfSb6/Ae8ZMefoELl3m4sSTlNanQ4W1nhuPaWDCO4Zc iz7xdqdMttvFlN4pR3jX17QVgCVS4IQ36VcQq4EGGwLLq5YqjRsdujJNxGkGbl3zwIQg nERXHj+AzQIYdskHMvmHGA8J+xE+GK0cIcItfwCvufvwETmn8rCFZ9fXrisQfaVz4YV/ +JqmvTQZaa38T6s8YOSOTWvFsx6SW4p/Cil2wX5WNCvP4IFLpY/69TeR4Q68D2CqcLAC H7wA== MIME-Version: 1.0 X-Received: by 10.180.188.35 with SMTP id fx3mr22169055wic.82.1408120933761; Fri, 15 Aug 2014 09:42:13 -0700 (PDT) Sender: asomers@gmail.com Received: by 10.194.86.225 with HTTP; Fri, 15 Aug 2014 09:42:13 -0700 (PDT) In-Reply-To: <3C86F281-D618-4B93-BBA3-2DA33AC407EC@worrbase.com> References: <3C86F281-D618-4B93-BBA3-2DA33AC407EC@worrbase.com> Date: Fri, 15 Aug 2014 10:42:13 -0600 X-Google-Sender-Auth: noPbHcctbzhqGeCxil608dckuLE Message-ID: Subject: Re: Inconsistent behavior with dd(1) From: Alan Somers To: William Orr Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: FreeBSD CURRENT X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Aug 2014 16:42:16 -0000 On Thu, Aug 14, 2014 at 11:55 PM, William Orr 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