Date: Sun, 9 Jun 2002 12:33:47 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Giorgos Keramidas <keramida@FreeBSD.ORG> Cc: audit@FreeBSD.ORG Subject: Re: badsect warns Message-ID: <20020609123105.X21758-100000@gamplex.bde.org> In-Reply-To: <20020609011418.GA70449@hades.hell.gr>
next in thread | previous in thread | raw e-mail | index | archive | help
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 majordomo@FreeBSD.org
with "unsubscribe freebsd-audit" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020609123105.X21758-100000>