cvs commit: src/sbin/badsect badsect.c

[Bruce Evans]

On Wed, 5 Jan 2005, Giorgos Keramidas wrote:

> On 2005-01-03 13:16, Scott Long <scottl at freebsd.org> wrote:
> >Robert Watson wrote:
> >>rwatson     2005-01-03 19:03:40 UTC
> >>
> >>  FreeBSD src repository
> >>
> >>  Modified files:
> >>    sbin/badsect         badsect.c
> >>  Log:
> >>  The badsect(8) utility uses atol(), which doesn't allow very good error
> >>  checking and only recognizes numbers in base 10.  The attached patch
> >>  checks errno after strtol() and uses a base of 0 to allow octal, or hex
> >>  sector numbers too.
> >>
> >>  PR:             73112
> >>  Submitted by:   keramida
> >>  MFC after:      2 weeks
> >>
> >>  Revision  Changes    Path
> >>  1.21      +4 -1      src/sbin/badsect/badsect.c
> >
> > I'd argue that atol() is buggy for a number of other reasons also,
> > mainly that it means that you're limited to 2^31 sectors.  Maybe this
> > should change to strtoull() ?
>
> Ideally, if that's not a problem, we should go for uintmax_t, since
> `number' in this case is then stored in a daddr_t, which is 64-bits
> wide.

Large block numbers still wouldn't actually work.  badsect might be
able to check that they are not in use, but when it tries to pass them
to mknod(2) it will find that they are too large and give up.  It must
give uo because bad block numbers must be representable as a dev_t and
must not equal NODEV.  Thus the range is currently limited to
[0..0xffffffffe] (32 bits unsigned for dev_t, less 1 value for NODEV).

badsect intentionally does the in-use check before the range check
since it used to be impossible for the range check to fail for a
physically possible block number so the range check never failed but
served as a bitrot check.  Now that the bits have rotted but are hard
to fix, there is little point in not doing an up-front range check and
just using strtoull().

The bits rotted when ffs2 made physical block numbers >= 2^31 possible.
The relevant changes to badsect for ffs2 support were just to change
daddr_t from 32 bits to 64 bits.  This might have allowed the in-use
check to work for block numbers between up to 2^63-1, but it could not
have extended the range of block numbers that would actually work to
any more than up to 2^32-2 (from 1TB to 2TB).

A previous lifetime of this bug started when disk sizes exceeded
32MB and ended when dev_t was expanded from 16 bits to 32.  Now we
have the same bug with the numbers changed from 2TB for disk sizes
and 32 bits for dev_t.  The previous lifetime was from approx. 1980
until 1994.  The length of this lifetime shows that badsect was rarely
used even when it was more needed 10-20 years ago.

Nearby bugs:
- there seems to be no check for block numbers < 0.  Negative block numbers
  might pass the in-use check; then when they are converted to a dev_t
  they break the overflow check for this conversion on machines with
  32-bit longs.
- mknod(2) is broken and accepts a dev_t of NODEV (0xffffffff), and IIRC
  converts it to a value of 0; 0 is an especially dangerous wrong
  number for a disk block.  badsect doesn't know about this bug so it
  doesn't try to avoid it.  A block number of NODEV can occur now that
  daddr_t is 64 bits, but only on 64-bit machines.  On 32-bit machines,
  a block number of NODEV is accidentally handled correctly as a side
  effect of breaking support for block numbers between 2^31 and 2^32-2
  because of the type mismatch between daddr_t and strtol(): strtol()
  is limited to 2^31-1 on 32-bit machines.  Using strtol() used to be
  perfectly correct since daddr_t is signed integral and long was the
  largest signed integral type, but C99 broke the latter.
- the result of strtol() is blindly assigned to a daddr_t.  This used to
  allow harmful overflow on 64-bit machines only -- daddr_t was 32 bits
  but the range checking for strtol() only limited the value to 2^63-1.
  It is dormant now that daddr_t is 64 bits and there are no 128 bit
  machines.

> How about something like this?  I could find no easy way to check for
> overflow of daddr_t, so this is still not quite perfect but it's an
> improvement that may help with Really-Huge(TM) disks.
>
> %%%
> Index: badsect.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/badsect/badsect.c,v
> retrieving revision 1.21
> diff -u -u -r1.21 badsect.c
> --- badsect.c	3 Jan 2005 19:03:40 -0000	1.21
> +++ badsect.c	5 Jan 2005 15:03:39 -0000
> @@ -62,6 +62,7 @@
>  #include <errno.h>
>  #include <dirent.h>
>  #include <fcntl.h>
> +#include <inttypes.h>
>  #include <libufs.h>
>  #include <paths.h>
>  #include <stdio.h>
> @@ -94,6 +95,7 @@
>  	DIR *dirp;
>  	char name[2 * MAXPATHLEN];
>  	char *name_dir_end;
> +	char *errp;

Style bugs:

(1) Unsorting of declarations.
(2) Declarations of the same type not on the same line.

>
>  	if (argc < 3)
>  		usage();
> @@ -124,8 +126,11 @@
>  			err(7, "%s", name);
>  	}
>  	for (argc -= 2, argv += 2; argc > 0; argc--, argv++) {
> -		number = strtol(*argv, NULL, 0);
> -		if (errno == EINVAL || errno == ERANGE)
> +		errp = NULL;

The variable pointed to by strto*()'s second parameter is an output
parameter; initializing it in the caller is bogus.

> +		errno = 0;

This initialization is necessary but is missing in the current version
(rev.1.21).

> +		number = strtoumax(*argv, &errp, 0);
> +		if (errno != 0 || errp != NULL || (errp != NULL &&

`errp' is guaranteed to be non-NULL here, so this check causes badsect
to always fail.  This bug may have been caused by the bad variable name
`errp'.  It is a "next" pointer, not an "error" pointer.

> +		    *errp != '\0' && *argv != NULL && **argv != '\0'))

If *argv is NULL, then we have already followed the null pointer in
strtoumax().  But it can't be null, since it is before the NULL at the
end of the args.  The **argv check has no effect (errp == *argv in
this case, so the check has already been done; anyway, this check is
strtoumax()'s responsibility and can't be done so simply since an
empty string for *argv is not the only possibility for a garbage arg).

A correct and portable strto*() check looks something like:

		if (errno != 0 || errp == *argv || *errp != '\0)

The current check is:

		if (errno == EINVAL || errno == ERANGE)

The (errp != *argv) part of the check can be written as (errno != EINVAL)
assuming POSIX extensions, but this just makes the code unportable and
takes 2 more bytes of source.  errno = EINVAL can also be set under POSIX
for unsupported bases.  Otherwise, the check in -current is incorrect in
omitting "*errp != '\0'" and verbose but not wrong in checking for a
limited set of errnos (it shouldn't check since it doesn't care about
details of the error, but EINVAL and ERANGE are the only possible values
in POSIX).

>  			err(8, "%s", *argv);
>  		if (chkuse(number, 1))
>  			continue;
> %%%

About checking for overflow of daddr_t: overflow of dev_t is checked by
casting to dev_t and checking if the value is unchanged.  For daddr_t,
this method can be used provided that the value is >= 0 (which needs to
be ensured earlier anyway).

Bruce