Re: badsect warns

On Sun, 9 Jun 2002, Giorgos Keramidas wrote:

> The following enables compiling badsect with WARNS=6.
> There seems to be something funny about fs->fs_size though.
> In <ufs/ffs/fs.h> it's not daddr_t but `long'.

It is actually int32_t, although it is logically a type a tiny bit
larger than ufs_daddr_t (ffs block numbers have type ufs_daddr_t and
the filesystem size is 1 larger than the largest ffs block number).
I think gcc is just warning about a sign mismatch now, but the cast
is almost necessary for correct code on machines with >= 32-bit
unsigned's.  It "fixes" overflow of the sum.

fs->fs_size has type int64_t in ufs2, so the cast is just broken
except on machines with >= 64-bit unsigneds.  ufs2 has similarly
broken casts internally, e.g., (u_int)bno in ffs_alloc.c.

> Is there some reason that this is cast to `unsigned'?

> %%%
> Index: badsect.c
> ===================================================================
> RCS file: /home/ncvs/src/sbin/badsect/badsect.c,v
> retrieving revision 1.13
> diff -u -r1.13 badsect.c
> --- badsect.c	16 May 2002 04:09:51 -0000	1.13
> +++ badsect.c	9 Jun 2002 01:00:54 -0000
> @@ -168,7 +168,7 @@
>  	daddr_t fsbn, bn;
>
>  	fsbn = dbtofsb(fs, blkno);
> -	if ((unsigned)(fsbn+cnt) > fs->fs_size) {
> +	if ((fsbn + cnt) > fs->fs_size) {
>  		printf("block %ld out of range of filesystem\n", (long)blkno);
>  		return (1);
>  	}
> %%%

The cast seems to be just the usual obsolete hack to check that a number
is >= 0 and <= a maximum value unsigned only one comparison:

	int foo, max:
	...
	if ((u_int)foo > max)
		err(...);

This is obsolete because non-primitive compilers will optimize the obvious
check:

	if (foo < 0 || foo > max)

into the above if that is an optimization.

It fails to do this correctly in lots of ways:
1) The unsigned cast must be to the same or a wider type than the left
   operand.
2) gcc tends to warn if the type of thr right operand is signed.
3) fsbn+cnt may have overflowed before we checked it.  Casting the terms
   of the sum would work better -- it prevents overflow provided the terms
   are nonnegative.  However, the terms are not known to be nonnegative here
   because the caller has not checked its (user-supplied!) input -- `cnt' is
   always 1, but fsbn is dbtofsb(fs, blkno) which is garbage if blkno is
   garbage, and blkno may be garbage since it is just atol(*argv).  This
   leads to bugs in the caller:
(4) Disk block numbers are read in using atol(), but this can only work if
   daddr_t has type long.  This was broken on machines with 64-bit longs,
   and ufs2 breaks it on machines with 32-bit longs.
(5) There is no overflow checking for atol() of course.
(6) Negative daddr_t's are errors in this context (probably in all contexts).
    They are not checked for, but the checking in chkuse() depends on them
    being weeded out earlier.

64-bit daddr_t's also break all the casts to long in error messages.  64-bit
daddr_t's can't be printed as longs if longs are 32 bits.

> If not, should we remove the cast and start building badsect with
> something more strict than WARNS=0?

Needs more extensive cleanups.  Warnings about sign mismatches basically
just acientally tell you about overlow of the top bit here.  Changing
daddr_t to 64 bits tends to cause overflow in 32 other bits.

Bruce


To Unsubscribe: send mail to [email protected]
with "unsubscribe freebsd-audit" in the body of the message