Re: svn commit: r204909 - head/sbin/newfs

[Bruce Evans]

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, &sectorsize);
+                       rval = expand_number_int(optarg, &sectorsize);
                        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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"