Date: Wed, 10 Mar 2010 04:47:11 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Maxim Sobolev <sobomax@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r204909 - head/sbin/newfs Message-ID: <20100310042436.D13688@delplex.bde.org> In-Reply-To: <201003091031.o29AV3JO088200@svn.freebsd.org> References: <201003091031.o29AV3JO088200@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100310042436.D13688>