mptutil, mfitil, expand_number

[Bruce Evans]

On Sat, 9 Nov 2013, Eitan Adler wrote:

> Any thoughts?

> diff --git a/lib/libutil/expand_number.3 b/lib/libutil/expand_number.3
> index f78223b..2f5871f 100644
> --- a/lib/libutil/expand_number.3
> +++ b/lib/libutil/expand_number.3
> @@ -51,12 +51,13 @@ argument.
> The
> .Fn expand_number
> function
> +is case-insensitive and
> follows the SI power of two convention.
> .Pp
> The prefixes are:
> .Bl -column "Prefix" "Description" "1000000000000000000" -offset indent
> .It Sy "Prefix" Ta Sy "Description" Ta Sy "Multiplier"
> -.It Li k Ta No kilo Ta 1024
> +.It Li K Ta No kilo Ta 1024
> .It Li M Ta No mega Ta 1048576
> .It Li G Ta No giga Ta 1073741824
> .It Li T Ta No tera Ta 1099511627776

This is consistent with bugs in what the function actually does.  The
prefixes should have been case-insensitive.  M should mean mega and
m milli.  It is hard to distinguish M meaning 1000**2 from M meaning
1024**2, but no one needs m meaning 1/1024 and this API can't represent
1/1024 (another design bug in the API is that it doesn't support floating
point).  k should mean 1000, and K should be available for 1024.  I
should blacklist anything that uses kibi or mebi.  Even this API doesn't
support them.

> diff --git a/usr.sbin/mptutil/mpt_config.c b/usr.sbin/mptutil/mpt_config.c
> index 17b9945..ae66258 100644
> --- a/usr.sbin/mptutil/mpt_config.c
> +++ b/usr.sbin/mptutil/mpt_config.c
> @@ -43,6 +43,7 @@ __RCSID("$FreeBSD$");
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> +#include <sysexits.h>

Using this is a style bug.

> #include <unistd.h>
> #include "mptutil.h"
>
> @@ -50,35 +51,6 @@ __RCSID("$FreeBSD$");
> static void dump_config(CONFIG_PAGE_RAID_VOL_0 *vol);
> #endif
>
> -static long
> -dehumanize(const char *value)
> -{
> -        char    *vtp;
> -        long    iv;
> -
> -        if (value == NULL)
> -                return (0);
> -        iv = strtoq(value, &vtp, 0);
> -        if (vtp == value || (vtp[0] != '\0' && vtp[1] != '\0')) {
> -                return (0);
> -        }

Using expand_number() gives different overflow bugs.

Here there was the usual null range checking for assiging quad_t to long.
There were more overflow bugs in the rest of the function.
expand_number() is better, but...

> /*
>  * Lock the volume by opening its /dev device read/write.  This will
>  * only work if nothing else has it opened (including mounts).  We
> @@ -666,7 +638,9 @@ create_volume(int ac, char **av)
>  quick = 1;
>  break;
>  case 's':
> - stripe_size = dehumanize(optarg);
> + error = expand_number(optarg, &stripe_size);

... stripe_size still has type long, so more than overflow occurs when
the range-checked uint64_t result of expand_number() is assigned to a
32-bit long.  The behaviour from the buffer overrun is undefined.
This fatal type mismatch should be detected at compile time on 32-bit
arches.

The not-so-fatal type mismatch of assigning an (unsigned) uint64_t to
a (signed) 64-bit long will probably not be detected at compile time.
This gives some overflow bugs.

> + if (error == -1)
> + errx(EX_USAGE, "%s: stripe size incorrect", optarg);
>  if ((stripe_size < 512) || (!powerof2(stripe_size))) {
>  warnx("Invalid stripe size %s", optarg);
>  return (EINVAL);

Something mangled this part of the diff.

The error handling is insistent.  The old code only warns and returns
EINVAL on error.  This function seems to never handle errors by exiting,
and that is a good design.

Something should check that the stripe size is not too large.
dehumanize() only limited it to LONG_MAX (modulo overflow bugs) and
expand_number() only limits it to UINT64_MAX (module overflow and other
bugs).  These values aren't powers of 2, so the upper limit is just
slightly smaller than them.

Using sysexits.h is not just a style bug.  It predjudices the (hopefully
better) error handling done in the caller (mptutil doesn't use sysexits
anywhere else), and loses the (hopefully more descriptive) errno returned
by expand_number().  expand_number() will return EINVAL on error (in errno)
in most cases too.  It is correct to use warnx() or errx() so as to not
print "Invalid" twice.  It would be useful to print something different
when expand_number() returns ERANGE, but this is not so easy.  Returning
ERANGE instead of EINVAL for this case is easy.

strtonum(3) has a different set of design errors and complications related
to all of this.  It does range checking that we want, but we can't use it
since it doesn't support scientific notation.  It generates strings
describing the range errors but these are hard to use; actually using its
complications makes it almost as hard to use as not using it.

Bruce