From owner-freebsd-current Sat Nov 16 19:36: 9 2002 Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id A8E9B37B401; Sat, 16 Nov 2002 19:36:05 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id EE6F243E42; Sat, 16 Nov 2002 19:36:03 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id OAA22296; Sun, 17 Nov 2002 14:35:52 +1100 Date: Sun, 17 Nov 2002 14:48:37 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Julian Elischer Cc: Tim Robbins , FreeBSD current users Subject: Re: Sign fixes for disklabel(8) In-Reply-To: Message-ID: <20021117135628.L21008-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG 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