From owner-svn-src-all@FreeBSD.ORG Sat Jan 22 07:47:48 2011 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5B055106564A; Sat, 22 Jan 2011 07:47:48 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id B967F8FC16; Sat, 22 Jan 2011 07:47:47 +0000 (UTC) Received: from c122-106-165-206.carlnfd1.nsw.optusnet.com.au (c122-106-165-206.carlnfd1.nsw.optusnet.com.au [122.106.165.206]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id p0M7liWx017034 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 22 Jan 2011 18:47:45 +1100 Date: Sat, 22 Jan 2011 18:47:44 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Maxim Sobolev In-Reply-To: <201101220521.p0M5LLc7081184@svn.freebsd.org> Message-ID: <20110122172226.R13556@besplex.bde.org> References: <201101220521.p0M5LLc7081184@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r217714 - head/sbin/fdisk X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 22 Jan 2011 07:47:48 -0000 On Sat, 22 Jan 2011, Maxim Sobolev wrote: > Log: > Warn user when value entered is greated than the amount supported > by the MBR for the given parameter and set that parameter to the > maximum value instead of just truncating the most significant part > silently. > > Could happen for example if the capacity of the device is more > than 2TB, so that the number of sectors is greater than 2Mib. > > MFC after: 1 month This improves some things, but has a high density of style bugs (many lines expanded beyond 80 columns...), and 2 errors in the limits. What's this 2Mib limit? ibits are strange units for sector counts. The limit should be 4G, but there is probably lots of brokenness starting at 2G. fdisk is quite broken at 2G, since it uses ints too much. > Modified: head/sbin/fdisk/fdisk.c > ============================================================================== > --- head/sbin/fdisk/fdisk.c Sat Jan 22 01:48:12 2011 (r217713) > +++ head/sbin/fdisk/fdisk.c Sat Jan 22 05:21:20 2011 (r217714) > @@ -586,9 +586,9 @@ change_part(int i) > tcyl = DPCYL(partp->dp_scyl,partp->dp_ssect); > thd = partp->dp_shd; > tsec = DPSECT(partp->dp_ssect); > - Decimal("beginning cylinder", tcyl, tmp); > - Decimal("beginning head", thd, tmp); > - Decimal("beginning sector", tsec, tmp); > + Decimal("beginning cylinder", tcyl, tmp, sizeof(partp->dp_scyl)); Cylinder numbers are 10 bits in the MBR. They don't fit in the starting and ending cylinder fields, so 2 bits of them are put in the corresponding sector fields. The sizeof here is always 1, giving only 8 bits and thus a wrong limit of 256. > + Decimal("beginning head", thd, tmp, sizeof(partp->dp_shd)); > + Decimal("beginning sector", tsec, tmp, sizeof(partp->dp_ssect)); Sector numbers are only 6 bits in the MBR. The sizeof here is always 1, giving 8 bits and thus a wrong limit of 256. > partp->dp_scyl = DOSCYL(tcyl); > partp->dp_ssect = DOSSECT(tsec,tcyl); > partp->dp_shd = thd; The DOSSECT() macro handles the details of merging cylinder bits into sector field.s Since these are the values to be written directly into the MBR, it is correct to limit them and not blindly truncate them. Non-blindly changing them is little better. They should just be rejected. There is also a lower limit on sector numbers. Sector 0 is invalid. Similarly for ending C/H/S numbers, except the breakage is larger in practice. Starting cylinder numbers are usually 0, but ending cylinder numbers are usually 1023 and now you can't edit them to anything above 255, including null changes from 1023 and changes from invalid values to the least invalid value of 1023. Older disks/devices/software may actually have between 256 and 1023 cylinders and need an MBR which matches. > @@ -647,7 +647,7 @@ change_active(int which) > setactive: > do { > new = active; > - Decimal("active partition", new, tmp); > + Decimal("active partition", new, tmp, 0); > if (new < 1 || new > 4) { > printf("Active partition number must be in range 1-4." > " Try again.\n"); This has a lower limit too, and does the necessary and correct range checking for itself. It now has to pass a bogus upper limit of 0 to Decimal() to stop checking there. Decimal() should probably handle both limits like this does. > @@ -677,9 +677,9 @@ get_params_to_use() > { > do > { > - Decimal("BIOS's idea of #cylinders", dos_cyls, tmp); > - Decimal("BIOS's idea of #heads", dos_heads, tmp); > - Decimal("BIOS's idea of #sectors", dos_sectors, tmp); > + Decimal("BIOS's idea of #cylinders", dos_cyls, tmp, 0); > + Decimal("BIOS's idea of #heads", dos_heads, tmp, 0); > + Decimal("BIOS's idea of #sectors", dos_sectors, tmp, 0); > dos_cylsecs = dos_heads * dos_sectors; > print_params(); > } I originally thought that the user values had to be accepted fairly blindly since they are multiplied out in places like here. Now I see that nothing has changed for these, but it probably should be changed to at least warn about them. There seems to be mo multiplication out involving dos_cyls. There are warnings about it being out of range all over the place. The size of the disk in sectors is mostly given not by these "dos" variables, but by the "undosed" variables cyls, heads and sectors. get_params() initializes all the "dos" variables badly by setting them to the same as to "undosed" variables in all cases. This is guranteed to give dos_cyls > 1023 on any hard disk less than about 12 years old. Note that the limits for the above are 1 more than the limits for starting and ending C/H/S, except for dos_sectors, since these values are counts while the the others are offsets from 0 (except starting/ending sectors are offset froms 1). > @@ -915,11 +915,16 @@ ok(const char *str) > } > > static int > -decimal(const char *str, int *num, int deflt) > +decimal(const char *str, int *num, int deflt, int size) > { > - int acc = 0, c; > + long long acc = 0, maxval; Long long is an abomination, and there is no reason to use any type larger than uint32_t here. > + int c; > char *cp; > > + if (size == 0) { > + size = sizeof(*num); > + } > + maxval = (long long)1 << (size * 8); This is not the maximum value, but a limit value which is 1 greater than the maximum. OK, you need a type larger than uint32_t to go 1 greater than its maximum, and you get that when `size' is 4. A better interface passing the maximum (_not_ 1 greater) wouldn't have that problem. And decimal() still returns int, so overflow occurs long before that UNIT32_MAX+1LL. Sector numbers are 32 bits unsigned, so int is inadequate to describe them, but this program uses int for them in many other places, e.g., 'disksecs = cyls * heads * sectors' first has an overflowing multiplication and then assigns the overflowed value to "int disksecs" where it won't fit :-(. > while (1) { > printf("Supply a decimal value for \"%s\" [%d] ", str, deflt); > fflush(stdout); > @@ -935,14 +940,20 @@ decimal(const char *str, int *num, int d > if (!c) > return 0; > while ((c = *cp++)) { > - if (c <= '9' && c >= '0') > - acc = acc * 10 + c - '0'; > - else > + if (c <= '9' && c >= '0') { > + if (acc < maxval) > + acc = acc * 10 + c - '0'; > + } else > break; > } Ugh, didn't people learn to use strtol() instead of home made parsing of numbers with overflow errors in 1985? Now it has enough range checking to prevent overflow of (acc * 10) provided maxval is limited to LONG_LONG_MAX / 10. Might need a slightly lower limit for adding c. decimal() doesn't seem to support negative numbers, so nothing needs to check for them. Bruce