From owner-freebsd-scsi@FreeBSD.ORG Sun Nov 10 06:25:41 2013 Return-Path: Delivered-To: scsi@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 ESMTP id DB0E7F34 for ; Sun, 10 Nov 2013 06:25:41 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 610DE28E1 for ; Sun, 10 Nov 2013 06:25:37 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 4EE261AA268; Sun, 10 Nov 2013 17:25:26 +1100 (EST) Date: Sun, 10 Nov 2013 17:25:25 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eitan Adler Subject: Re: mptutil, mfitil, expand_number In-Reply-To: Message-ID: <20131110161359.S1474@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=bpB1Wiqi c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=WvzHK2nvTcUA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=oiRXuiCKKlcA:10 a=eR1LQOD-GpsQidJ1S0gA:9 a=XtSjqWxeeNbtF_oK:21 a=REZHW0Y7hR6k14Dp:21 a=CjuIK1q_8ugA:10 Cc: scsi@FreeBSD.org X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Nov 2013 06:25:41 -0000 On Sat, 9 Nov 2013, Eitan Adler wrote: > Any thoughts? > diff --git a/lib/libutil/expand_number.3 b/lib/libutil/expand_number.3 > index f78223b..2f5871f 100644 > --- a/lib/libutil/expand_number.3 > +++ b/lib/libutil/expand_number.3 > @@ -51,12 +51,13 @@ argument. > The > .Fn expand_number > function > +is case-insensitive and > follows the SI power of two convention. > .Pp > The prefixes are: > .Bl -column "Prefix" "Description" "1000000000000000000" -offset indent > .It Sy "Prefix" Ta Sy "Description" Ta Sy "Multiplier" > -.It Li k Ta No kilo Ta 1024 > +.It Li K Ta No kilo Ta 1024 > .It Li M Ta No mega Ta 1048576 > .It Li G Ta No giga Ta 1073741824 > .It Li T Ta No tera Ta 1099511627776 This is consistent with bugs in what the function actually does. The prefixes should have been case-insensitive. M should mean mega and m milli. It is hard to distinguish M meaning 1000**2 from M meaning 1024**2, but no one needs m meaning 1/1024 and this API can't represent 1/1024 (another design bug in the API is that it doesn't support floating point). k should mean 1000, and K should be available for 1024. I should blacklist anything that uses kibi or mebi. Even this API doesn't support them. > diff --git a/usr.sbin/mptutil/mpt_config.c b/usr.sbin/mptutil/mpt_config.c > index 17b9945..ae66258 100644 > --- a/usr.sbin/mptutil/mpt_config.c > +++ b/usr.sbin/mptutil/mpt_config.c > @@ -43,6 +43,7 @@ __RCSID("$FreeBSD$"); > #include > #include > #include > +#include Using this is a style bug. > #include > #include "mptutil.h" > > @@ -50,35 +51,6 @@ __RCSID("$FreeBSD$"); > static void dump_config(CONFIG_PAGE_RAID_VOL_0 *vol); > #endif > > -static long > -dehumanize(const char *value) > -{ > - char *vtp; > - long iv; > - > - if (value == NULL) > - return (0); > - iv = strtoq(value, &vtp, 0); > - if (vtp == value || (vtp[0] != '\0' && vtp[1] != '\0')) { > - return (0); > - } Using expand_number() gives different overflow bugs. Here there was the usual null range checking for assiging quad_t to long. There were more overflow bugs in the rest of the function. expand_number() is better, but... > /* > * Lock the volume by opening its /dev device read/write. This will > * only work if nothing else has it opened (including mounts). We > @@ -666,7 +638,9 @@ create_volume(int ac, char **av) > quick = 1; > break; > case 's': > - stripe_size = dehumanize(optarg); > + error = expand_number(optarg, &stripe_size); ... stripe_size still has type long, so more than overflow occurs when the range-checked uint64_t result of expand_number() is assigned to a 32-bit long. The behaviour from the buffer overrun is undefined. This fatal type mismatch should be detected at compile time on 32-bit arches. The not-so-fatal type mismatch of assigning an (unsigned) uint64_t to a (signed) 64-bit long will probably not be detected at compile time. This gives some overflow bugs. > + if (error == -1) > + errx(EX_USAGE, "%s: stripe size incorrect", optarg); > if ((stripe_size < 512) || (!powerof2(stripe_size))) { > warnx("Invalid stripe size %s", optarg); > return (EINVAL); Something mangled this part of the diff. The error handling is insistent. The old code only warns and returns EINVAL on error. This function seems to never handle errors by exiting, and that is a good design. Something should check that the stripe size is not too large. dehumanize() only limited it to LONG_MAX (modulo overflow bugs) and expand_number() only limits it to UINT64_MAX (module overflow and other bugs). These values aren't powers of 2, so the upper limit is just slightly smaller than them. Using sysexits.h is not just a style bug. It predjudices the (hopefully better) error handling done in the caller (mptutil doesn't use sysexits anywhere else), and loses the (hopefully more descriptive) errno returned by expand_number(). expand_number() will return EINVAL on error (in errno) in most cases too. It is correct to use warnx() or errx() so as to not print "Invalid" twice. It would be useful to print something different when expand_number() returns ERANGE, but this is not so easy. Returning ERANGE instead of EINVAL for this case is easy. strtonum(3) has a different set of design errors and complications related to all of this. It does range checking that we want, but we can't use it since it doesn't support scientific notation. It generates strings describing the range errors but these are hard to use; actually using its complications makes it almost as hard to use as not using it. Bruce