svn commit: r217714 - head/sbin/fdisk

[Warner Losh]

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
>
>
>