Date: Mon, 10 Jan 2005 03:36:11 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Giorgos Keramidas <keramida@freebsd.org> Cc: cvs-src@freebsd.org Subject: Re: cvs commit: src/sbin/badsect badsect.c Message-ID: <20050110003627.T28619@delplex.bde.org> In-Reply-To: <20050105150917.GA1592@orion.daedalusnetworks.priv> References: <200501031903.j03J3eBd044669@repoman.freebsd.org> <20050105150917.GA1592@orion.daedalusnetworks.priv>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 5 Jan 2005, Giorgos Keramidas wrote:
> On 2005-01-03 13:16, Scott Long <scottl@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20050110003627.T28619>