On Wed, Mar 23, 2011 at 02:22:58PM +0100, David Coppa wrote:

> On Tue, 18 Jan 2011, Vadim Zhukov wrote:
> 
> > Hello all (again).
> > 
> > So here is a bit reworked patch, taking into account suggestions from
> > tedu@, halex@ and jmc@. Particularily, there is no more huge phrase
> > "This value is multiplied by the number of 512\-byte blocks in a sector
> > to yield the size...": just sectors being converted to (512-byte)
> > blocks. I added explicit checks for the sector size be multiply of 512
> > as it's what the kernel does - but the kernel does not print such nice
> > error messages and, we need this for our math being correct anyway.
> > 
> > In addition to all suggestions I also fixed ambiguity in mkfs.c,
> > adding word "sectors" to error message.
> > 
> > Thanks to all reviewers. Hope this'll be commited.
> 
> Has this been forgotten?
> 
> I'm using it since January and it works fine:
> 
> $ grep mfs /etc/fstab
> 4d700a703f68291d.b /tmp mfs rw,nodev,nosuid,-s=256m 0 0
> 
> $ mount | grep mfs
> mfs:5635 on /tmp type mfs (asynchronous, local, nodev, nosuid, size=524288 
> 512-blocks)
> 
> 
> Cheers, 
> David

I think there a bug, see inline.

> Index: newfs.c
> ===================================================================
> RCS file: /cvs/src/sbin/newfs/newfs.c,v
> retrieving revision 1.88
> diff -u -p -r1.88 newfs.c
> --- newfs.c   13 Dec 2010 00:02:58 -0000      1.88
> +++ newfs.c   18 Jan 2011 15:47:45 -0000
> @@ -114,7 +114,7 @@ int       mfs;                    /* run as the memory 
> based fi
>  int  Nflag;                  /* run without writing file system */
>  int  Oflag = 1;              /* 0 = 4.3BSD ffs, 1 = 4.4BSD ffs, 2 = ffs2 */
>  daddr64_t    fssize;                 /* file system size */
> -int  sectorsize;             /* bytes/sector */
> +long long    sectorsize;             /* bytes/sector */
>  int  fsize = 0;              /* fragment size */
>  int  bsize = 0;              /* block size */
>  int  maxfrgspercg = INT_MAX; /* maximum fragments per cylinder group */
> @@ -169,6 +169,8 @@ main(int argc, char *argv[])
>       char **saveargv = argv;
>       int ffsflag = 1;
>       const char *errstr;
> +     long long fssize_input;

fssize_input is not initialized.

> +     int fssize_usebytes = 0;
>  
>       if (strstr(__progname, "mfs"))
>               mfs = Nflag = quiet = 1;
> @@ -192,9 +194,9 @@ main(int argc, char *argv[])
>                       oflagset = 1;
>                       break;
>               case 'S':
> -                     sectorsize = strtonum(optarg, 1, INT_MAX, &errstr);
> -                     if (errstr)
> -                             fatal("sector size is %s: %s", errstr, optarg);
> +                     if (scan_scaled(optarg, &sectorsize) == -1 ||
> +                         sectorsize <= 0 || (sectorsize % DEV_BSIZE))
> +                             fatal("sector size invalid: %s", optarg);
>                       break;
>               case 'T':
>                       disktype = optarg;
> @@ -264,11 +266,17 @@ main(int argc, char *argv[])
>               case 'q':
>                       quiet = 1;
>                       break;
> +
>               case 's':
> -                     fssize = strtonum(optarg, 1, LLONG_MAX, &errstr);
> -                     if (errstr)
> -                             fatal("file system size is %s: %s",
> -                                 errstr, optarg);
> +                     if (scan_scaled(optarg, &fssize_input) == -1 ||
> +                         fssize_input <= 0)
> +                             fatal("file system size invalid: %s", optarg);
> +                     fssize_usebytes = 0;    /* in case of multiple -s */
> +                     for (s1 = optarg; *s1 != '\0'; s1++)
> +                             if (isalpha(*s1)) {
> +                                     fssize_usebytes = 1;
> +                                     break;
> +                             }
>                       break;

fssize_input is only set if -s is specified. 

>               case 't':
>                       fstype = optarg;
> @@ -412,17 +420,25 @@ main(int argc, char *argv[])
>                             argv[0], *cp);
>       }
>  havelabel:
> -     if (fssize == 0)
> -             fssize = DL_GETPSIZE(pp);
> -     if (fssize > DL_GETPSIZE(pp) && !mfs)
> -            fatal("%s: maximum file system size on the `%c' partition is 
> %lld",
> -                     argv[0], *cp, DL_GETPSIZE(pp));
> -
>       if (sectorsize == 0) {
>               sectorsize = lp->d_secsize;
>               if (sectorsize <= 0)
>                       fatal("%s: no default sector size", argv[0]);
>       }
> +
> +     if (fssize_usebytes) {
> +             fssize = (daddr64_t)fssize_input / (daddr64_t)sectorsize;
> +             if ((daddr64_t)fssize_input % (daddr64_t)sectorsize != 0)
> +                     fssize++;
> +     } else if (fssize_input == 0)

Uninitialized access of fssize_input if fssize_usebytes == 0 and no -s
was given. 

        -Otto

> +             fssize = DL_GETPSIZE(pp);
> +     else
> +             fssize = (daddr64_t)fssize_input;
> +
> +     if (fssize > DL_GETPSIZE(pp) && !mfs)
> +            fatal("%s: maximum file system size on the `%c' partition is "
> +                "%lld sectors", argv[0], *cp, DL_GETPSIZE(pp));
> +
>       fssize *= sectorsize / DEV_BSIZE;
>       if (oflagset == 0 && fssize >= INT_MAX)
>               Oflag = 2;      /* FFS2 */

Reply via email to