Date: Sun, 23 Jan 2011 23:25:52 -0700 From: Warner Losh <imp@bsdimp.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, Maxim Sobolev <sobomax@FreeBSD.org>, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r217714 - head/sbin/fdisk Message-ID: <4D3D1B70.8020405@bsdimp.com> In-Reply-To: <20110122172226.R13556@besplex.bde.org> References: <201101220521.p0M5LLc7081184@svn.freebsd.org> <20110122172226.R13556@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Maxim, In case it wasn't clear from Bruce's long explaination, parts of this are technically wrong and need to be reverted or fixed. Warner On 01/22/2011 00:47, Bruce Evans wrote: > 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 > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4D3D1B70.8020405>