From owner-svn-src-all@FreeBSD.ORG Tue Mar 9 17:47:20 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 697CB1065670; Tue, 9 Mar 2010 17:47:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail08.syd.optusnet.com.au (mail08.syd.optusnet.com.au [211.29.132.189]) by mx1.freebsd.org (Postfix) with ESMTP id F36FF8FC12; Tue, 9 Mar 2010 17:47:19 +0000 (UTC) Received: from c220-239-227-59.carlnfd1.nsw.optusnet.com.au (c220-239-227-59.carlnfd1.nsw.optusnet.com.au [220.239.227.59]) by mail08.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o29HlBx1008199 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 10 Mar 2010 04:47:17 +1100 Date: Wed, 10 Mar 2010 04:47:11 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Maxim Sobolev In-Reply-To: <201003091031.o29AV3JO088200@svn.freebsd.org> Message-ID: <20100310042436.D13688@delplex.bde.org> References: <201003091031.o29AV3JO088200@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r204909 - head/sbin/newfs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Mar 2010 17:47:20 -0000 On Tue, 9 Mar 2010, Maxim Sobolev wrote: > Log: > Change secrorsize back to int, since that's the data type expected by the > ioctl(DIOCGSECTORSIZE). It creates issues on some architectures. This is how it should have been done originally. Except, it should have been done for all the int variables, and without the bugs in this version. > Modified: head/sbin/newfs/newfs.c > ============================================================================== > --- head/sbin/newfs/newfs.c Tue Mar 9 06:43:35 2010 (r204908) > +++ head/sbin/newfs/newfs.c Tue Mar 9 10:31:03 2010 (r204909) > @@ -92,7 +92,7 @@ int Jflag; /* enable gjournal for file > int lflag; /* enable multilabel for file system */ > int nflag; /* do not create .snap directory */ > intmax_t fssize; /* file system size */ > -int64_t sectorsize; /* bytes/sector */ > +int sectorsize; /* bytes/sector */ > int realsectorsize; /* bytes/sector in hardware */ > int64_t fsize = 0; /* fragment size */ > int64_t bsize = 0; /* block size */ > @@ -119,6 +119,7 @@ static void getfssize(intmax_t *, const > static struct disklabel *getdisklabel(char *s); > static void rewritelabel(char *s, struct disklabel *lp); > static void usage(void); > +static int expand_number_int(const char *buf, int *num); Style bug: insertion sort error. The prototypes used to be sorted, but the order had already rotted with getfssize(). > @@ -171,7 +172,7 @@ main(int argc, char *argv[]) > Rflag = 1; > break; > case 'S': > - rval = expand_number(optarg, §orsize); > + rval = expand_number_int(optarg, §orsize); > if (rval < 0 || sectorsize <= 0) > errx(1, "%s: bad sector size", optarg); > break; As pointed out in another reply, sectorsize should be u_int in another context (for DICOGSECTORSIZE), but here negative values and values larger than INT_MAX are disallowed so there is no problem here. There is no range checking for the value returned by DIOCGSECTORSIZE. It is not really needed since the kernel shouldn't return an invalid sector size. The range checking here is rather pointless since sector sizes of <= 0 are only some of the ones that won't work. Ones like 42 won't work either. No non-power of 2 will work, and huge values won't work. There may be checks for huge values later. > @@ -496,3 +497,20 @@ usage() > fprintf(stderr, "\t-s file system size (sectors)\n"); > exit(1); > } > + > +static int > +expand_number_int(const char *buf, int *num) > +{ > + int64_t num64; > + int rval; > + > + rval = expand_number(buf, &num64); > + if (rval != 0) > + return (rval); This is missing the style bug of testing for (rval < 0). > + if (num64 > INT_MAX) { > + errno = ERANGE; > + return (-1); > + } This is missing a check for (num64 < INT_MIN). > + *num = (int)num64; This overflows when num64 < INT_MIN. The overflow normally results in truncation. If the truncated value is > 0, the error is not detected later. > + return (0); > +} Extending expand_number_int() and expand_number() to support unsigned values is not easy without bloating the API. expand_number_int() is broken as designed, since it doesn't support unsigned values or [u]intmax_t values. strtonum() is even more broken as designed. Bruce