Date: Sat, 16 Nov 2002 20:07:55 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Tim Robbins <tjr@FreeBSD.ORG> Cc: Julian Elischer <julian@elischer.org>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Sign fixes for disklabel(8) Message-ID: <20021116194439.E17742-100000@gamplex.bde.org> In-Reply-To: <20021116131842.A92191@dilbert.robbins.dropbear.id.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 16 Nov 2002, Tim Robbins wrote:
> On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
>
> > Here are the diffs to allow disklabel to correctly create partitions >
> > 1TB (up to 2TB is useful with UFS2) pending a different partitionning
> > scheme. It also allows you to correctly make smaller partitions beyond
> > 1TB which is nice if you don't want to waste 800GB on an array :-)
Labels should be able to handle disks much larger than 1TB (under -current
where there is no significant daddr_t limit) by fudging the sector size.
E.g., with a sector size of 8K should work up to 16TB (or 8TB without your
fixes) without even requiring larger ffs block sizes than the default of
16K. (I used 8K instead of 16K in this example because ffs has problems
reading the superblock starting at 8K.)
> [...]
> > - int v, lineno = 0, errors = 0;
> > + unsigned int v;
Style bug: "unsigned int" is spelled u_int everywhere else in disklabel.c
except in one place (which shouldn't even use an unsigned type).
Badly chosen type: 'v' is general purpose; it should have type u_long if
anything, though it wants to be uint32_t for sector counts.
> > + int lineno = 0, errors = 0;
> > int i;
> [...]
> > - v = atoi(tp);
> > + v = strtoul(tp, NULL, 0);
This code is almost as low-quality as before. The following errors are
not checked for:
- garbage input
- overflow inside strtoul()
- overflow on assignment of the result of strtoul() to v (since we chose
a poor type for v). The old poorly chosen type was at least suitable
for holding the result of atoi().
> > if ((unsigned)v >= DKMAXTYPES)
> > fprintf(stderr, "line %d:%s %d\n", lineno,
> > "Warning, unknown disk type", v);
>
> This cast is redundant since v is already unsigned. The fprintf() format
> string needs to use %u instead of %d.
But v really wants to be a plain int here.
> Use 10 as the base argument to
> stroul(), not 0 otherwise numbers with leading zeros will be interpreted
> as octal.
> [...]
> > - v = atoi(tp);
> > + v = strtoul(tp, NULL, 0);
> > if (v <= 0 || (v % DEV_BSIZE) != 0) {
> > fprintf(stderr,
> > "line %d: %s: bad sector size\n",
>
> Should be == 0, not <= 0 since v is unsigned.
>
> [...]
> > - v = atoi(tp);
> > + v = strtoul(tp, NULL, 0);
> > if (v < 0) {
> > fprintf(stderr, "line %d: %s: bad %s\n",
> > lineno, tp, cp);
>
> v < 0 is impossible.
These v's really want to be plain ints too. The old code was correct for
all these v's, modulo the usual sloppy parsing giving by atoi().
The sloppy parsing shouldn't be fixed now. Just use a different variable
and different parsing for the sector counts.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021116194439.E17742-100000>