Date: Sat, 3 Jun 1995 14:03:19 +1000 From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org, terry@cs.weber.edu Subject: Re: Some notes on UFS Message-ID: <199506030403.OAA31672@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>I've been looking arounf for 64 bit clean/unclean, and here's what Iive >found (so far): >ufs_bmap.c: > line 152: is_sequential(ump, ip->i_db[ ... > > because the is_sequential macro forces evaluation, there should be > an explicit cast in the macro when doing address aritmatic to ensure > the sign of the values being compared (address cast), since addresses > can be compared as signed values. This is definitely a nit. The > is_sequential macro is in ufs_mount.h. Erm, addresses are always compared as unsigned values, there are no addresses involved (only positive daddr_t values), and the comparison is for equality anyway. > line 201: for (bn = xap->in_off + 1; ... > using bn ( a daddr_t: types.h: long) this way is potentially wrong; > I don't know if it should be passed into bmaparray as unsigned in > the first place or not (comparing against ump->um_nindir: MNINDIR, > an unsigned long value: ufs_mount.h is definitely bad). This depends on whether daddr_t is supposed to be for the in-core or the on-disk representation of block numbers. It is currently supposed to be for both, but this is impossible in general (there may be no type with the same size as the on-disk object, and the on-disk object may have the bytes in a different order...). I think the correct approach is for daddr_t to be the in-core type and for the on-disk object to be converted iff necessary, but this takes a lot of work; it's easier to specify a compiler that supports a 32 bit type and abuse this type no matter how inefficient it is. >ufs_disksubr.c: > in the routine diskerr(): > the bp->b_bcount vs. lp->d_secsize is signed vs. unsigned. > This points up a potential problem in struct buf, with b_bcount > being a long (else why have d_secsize unsigned?). I don't think there are any actual problems here. b_bcount and d_secsize are always relatively small positive values. Even 16-bit signed ints could be used for them without losing much. I prefer to use unsigned ints for all such counters, but there are advantages to using [long] ints in sloppy code that doesn't worry about underflow or overflow: underflow from 0 to -1 is often useful, and the compiler is allowed to check for overflow of ints. >ufs_lookup.c: > line 158: int vpid; > This should be: u_long vpid; Yes. But do we really want 64-bit vpids if u_longs are 64 bits? > line 244: endsearch = roundup(dp->i_size, DIRBLKSIZ); > endsearch is a doff_t (inode.h: unsigned long); the conversion from > a quad (dp->i_size) should be explicit (via a macro from inode.h > where long is used instead of quad for doff_t typedef?). The conversion is wrong iff the directory is larger than 2G :-). There are more important overflows to worry about. > line 387: dp->i_offset = roundup(dp->i_size, DIRBLKSIZ); > same conversion as @ 244. Similarly. I don't think directories larger than 2G are worth worrying about. This is probably best enforced at directory extension time. > line 665: newdir.d_namlen = cnp->cn_namelen; > cn_namelen (namei.h: long) seems to be a long for no good reason? > otherwise, this is a potential loss of precision. It should probably have type size_t to avoid gratuitous conversions after strlen is used. However perhaps it needs to be signed so that it can be counted down from 0 to -1. > line 763: if (spacefree + dsize < newentrysize) > dsize is an unsigned int, while the other two variables are int. > According to ANSI, the lvalue of the addition is unsigned, so the > compare for an unsigned less than a signed is probably buggy at > the boundry conditions. I think all the values here are <= the block size so there are no problems. The mixture of types is just ugly. If there is a bug, then it is not checking for overflow of the addition. > line 825: ep->d_reclen += dp->i_reclen; > d_reclen (dirent.h: unsigned short) is potentially smaller than > i_reclen (inode.h: unsigned long), so this is a potential overflow. Even int + int can overflow. I try not to worry about this too much :-). > line 878: for (off = 0; off < ip->i_size; ... > > off is an off_t (signed quad) and i_size is an alias for i_din.di_size > which is (dinode.h: u_quad_t) an unsigned quad. This is probably > a nit. Another problem with directories larger than 2G. > line 941: if (target->i_number == rootino) > If this isn't going to be referenced as a manifest constant (ROOTINO) > for no good reason, at least declare it as an ino_t instead of as > an int (ino_t: types.h: unsigned long). > line 972: if (dirbuf.dotdot_ino == rootino) > same problem here. Yes. >ufs_quota.c: > line 169: ncurblocks should be declared unsigned. > line 285: ncurinodes should be declared unsigned. Yes (u_long). >ufs_vnops.c: > line 769: oldparent, newparent should be ino_t, not int. Yes. > line 1474: ...VTOI(ap->a_vp)->i_size <= uio->uio_offset; > i_size is unsigned, uio_offset is signed (off_t). Not a problem. Files >= 2^63 bytes are not supported. > line 1494: isize = ip->i_size; > since a path can not exceed MAXPATHLEN, this should be an explicit > cast. The int = quad here is an intentional loss of precision. Maybe a damaged symlink can be larger than 2G :-). Summary: the problems reported aren't serious yet. I'm sure there are thousands of similar problems and that they could best be found by the compiler, but the poorly chosen types should result in hundreds of thousands of warnings about suspicious conversions. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199506030403.OAA31672>