Date: Sun, 17 Nov 2002 14:48:37 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Julian Elischer <julian@elischer.org> Cc: Tim Robbins <tjr@FreeBSD.ORG>, FreeBSD current users <current@FreeBSD.ORG> Subject: Re: Sign fixes for disklabel(8) Message-ID: <20021117135628.L21008-100000@gamplex.bde.org> In-Reply-To: <Pine.BSF.4.21.0211161329240.53326-100000@InterJet.elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 16 Nov 2002, Julian Elischer wrote:
> On Sat, 16 Nov 2002, Bruce Evans wrote:
>
> > On Sat, 16 Nov 2002, Tim Robbins wrote:
> >
> > > On Fri, Nov 15, 2002 at 03:59:25PM -0800, Julian Elischer wrote:
> > 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.
> Unfortunatly the fdisk label then becomes a problem for compatible
> machines. The standard for ia64 machines looks like being hte way to go
> and we already have tools a code to handle it..
I think it is backwards and sideways compatible a long way, modulo bugs,
since the sector size is part of the label data -- it serves as a scale
factor for all the sector counts in the label. Old version of FreeBSD
certainly had bugs in this area (they confused the sector size with
DEV_BSIZE).
> > > [...]
> > > > - int v, lineno = 0, errors = 0;
> > > > + unsigned int v;
> >
> > Style bug: "unsigned int" is spelled u_int everywhere else in disklabel.c
>
> is it compulsary? "unsigned int" is standard C.
Not compulsory. I normally use it always in the kernel but only use it in
applications if the application already uses it (or other u_foo's).
> "u_int" is used in exactly one place in disklabel.c where
> "unsigned" occured in many so to maintain style I should change the one
> u_int to "unsigned int".
Oops, I grepped for u_ and found lots. They are mainly u_long's. It
is normal for u_long's to be much more common that u_int's because
half-baked attempts at portability involve using longs (strictly, ints
should not be used for values which might be larger than 32767). The
main reason that disklabel has lots of u_longs is that struct disklabel
used u_long for most of its struct members. This was bogotified by
changing all the u_long's in struct disklabel to u_int32_t's without
changing even one of the u_long's in disklabel.c.
> I'm happy to change to u_int though. (it's shorter)
THat is the main reason to use u_foo.
> > except in one place (which shouldn't even use an unsigned type).
>
> where?
Here in getasciilabel():
% unsigned int part;
% ...
% /* Process a partition specification line. */
% part = *cp - 'a';
`part' wants to be a small integer (between 0 and 'h' - 'a'). Note that
the above subtraction overflows if *cp < 'a'.
% if (part >= lp->d_npartitions) {
We use the unsignedness of `part' here (only) to avoid an explicit check
that part < 0 (the unsigned hack/obfuscation). (Note that
lp->d_npartitions has type u_int16_t, so it only gives the hack on 16-bit
machines.)
> > 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.
>
> do you mind if I don't rewrite the whole program?
> 'v' works so I'll leave it.
I'm trying to get you to not rewrite it! You rewrote everything that uses
`v', reducing the quality of most users of 'v' to an even lower level.
> > > [...]
> > > > - v = atoi(tp);
> > > > + v = strtoul(tp, NULL, 0);
> >
> > This code is almost as low-quality as before. The following errors are
> > not checked for:
>
> "almost" but not worse.. and now it works, for the criteria I'm
> looking for (2TB drives)..
It is worse. 'v' now has type unsigned int, so overflow may occur in
the program on assignment of the u_long value returned by strtoul().
Previously, overflow may have occurred in atoi() instead, since atoi()
has a similarly low quality. It is standard that atoi() has low quality
(its behaviour for unrepresentable values is undefined), so disklabel.c
could be blamed for using it, but the low quality is more painfully
obvious when it is in the program.
> > - 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().
>
> The new one is suitable for strtoul() which is alll that is used now.
u_long would be suitable for strtoul().
>
> >
> > > > 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.
>
> why? all teh types are +ve and lp->d_type is unsigned.
Same reason that file descriptors have type int. Using unsigned types
to leads to a morass of (un)sign extension and (non-)overflow problems.
They shouldn't be used just because the values are small and positive.
lp->d_type actually has type u_int16_t which promotes to plain int on
machines with 32-bit ints.
> > The sloppy parsing shouldn't be fixed now. Just use a different variable
> > and different parsing for the sector counts.
>
> sector counts can not be -ve so it should be unsigned..
No, they should be unsigned because we are seriously short of bits. We
don't actually want unsigned arithmetic for sector counts, since we
want expressions like (sector_count_t)(0xFFFFFFFF + 1) (where 0xFFFFFFFF
is the largest possible sector number) to be an error and not wrap to 0.
> I may substitute a u_int16_t variable for some.
I hope you mean u_int32_t. u_long is still best for working with the
value returned by strtoul(). Assign that value to the u_int32_t sector
count fields in the label only after checking that it fits. I think
a complete range check for these fields should be done now that we are
bumping into their limits. But please limit the changes to the sector
count fields unless you want to do them all right.
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?20021117135628.L21008-100000>