svn commit: r217714 - head/sbin/fdisk

[Bruce Evans]

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