svn commit: r236917 - head/sys/kern

Bruce Evans brde at optusnet.com.au
Tue Jun 12 02:53:54 UTC 2012


On Mon, 11 Jun 2012, Pawel Jakub Dawidek wrote:

> Log:
>  Use consistent way of checking if descriptor number is valid.
>
>  MFC after:	1 month
>
> Modified:
>  head/sys/kern/kern_descrip.c
>
> Modified: head/sys/kern/kern_descrip.c
> ==============================================================================
> --- head/sys/kern/kern_descrip.c	Mon Jun 11 20:12:13 2012	(r236916)
> +++ head/sys/kern/kern_descrip.c	Mon Jun 11 20:17:20 2012	(r236917)
> @@ -245,7 +245,7 @@ fd_last_used(struct filedesc *fdp, int l
> static int
> fdisused(struct filedesc *fdp, int fd)
> {
> -        KASSERT(fd >= 0 && fd < fdp->fd_nfiles,
> +        KASSERT((unsigned int)fd < fdp->fd_nfiles,
>             ("file descriptor %d out of range (0, %d)", fd, fdp->fd_nfiles));
> 	return ((fdp->fd_map[NDSLOT(fd)] & NDBIT(fd)) != 0);
> }

This is backwards.  Apart from using the worst possible (most verbose)
spelling of `unsigned', it uses a type hack manually optimize away the
test for fd being < 0.  The compiler will do this "optimization"
automatically if it is any good (or undo it if it is not), so all the
hack does is obfuscate the test.  With the verbose spelling of u_int,
it even takes more space.

> @@ -2212,7 +2212,7 @@ fget_unlocked(struct filedesc *fdp, int
> 	struct file *fp;
> 	u_int count;
>
> -	if (fd < 0 || fd >= fdp->fd_nfiles)
> +	if ((unsigned int)fd >= fdp->fd_nfiles)
> 		return (NULL);
> 	/*
> 	 * Fetch the descriptor locklessly.  We avoid fdrop() races by
> @@ -2598,7 +2598,7 @@ dupfdopen(struct thread *td, struct file
> 	 * closed, then reject.
> 	 */
> 	FILEDESC_XLOCK(fdp);
> -	if (dfd < 0 || dfd >= fdp->fd_nfiles ||
> +	if ((unsigned int)dfd >= fdp->fd_nfiles ||
> 	    (wfp = fdp->fd_ofiles[dfd]) == NULL) {
> 		FILEDESC_XUNLOCK(fdp);
> 		return (EBADF);
>

Similarly.  Old code from 4.4BSD uses the hack quite often, but this was
mostly fixed:

4.4BSD-Lite2 kern_descrip.c:
     12 instances of u_int
     7 of the 12 for this type hack
     0 instances where `unsigned' is misspelled as itself
     0 instances where `unsigned' is misspelled as `unsigned int'

FreeBSD-3 kern_descrip.c:
     7 instances of u_int
     1 of the 7 for this type hack
     8 instances where `unsigned' is misspelled as itself
     8 of the 8 for this type hack
     0 instances where `unsigned' is misspelled as `unsigned int'

     The apparent regression from u_int to unsigned was actually a fix in
     Lite2:
     - Lite1 misspelled `unsigned' as itself, except in 1 of the type hacks
     - FreeBSD-3 was essentially the Lite1 version
     - Lite2 changed everything to use the correct spelling.

FreeBSD-4 kern_descrip.c:
     Same as FreeBSD-4 (it hadn't caught up with Lite2).

FreeBSD-5 kern_descrip.c:
     5 instances of u_int
     0 of the 5 for this type hack
     3 instances where `unsigned' is misspelled as itself
     3 of the 3 for this type hack
     1 of the 3 misformatted with a space after the cast
     0 instances where `unsigned' is misspelled as `unsigned int'
     Very few instances where an fd is compared with '< 0'.  Just some
     where the upper limit is not checked nearby, so the type hack doesn't
     even apply.

     Some instances moved to filedesc.h.  There seems to be only 1 there
     now.  You regressed this too, back to worse than it was in Lite1
     (it now uses `unsigned int', but regression to Lite1 would have made
     it use `unsigned').  It used to be correct, but it took several rounds
     of churning to fix it there:
     - when fget_locked() was first moved there in 2002, it had a bogus
       cast to u_int on 1 of the fd's, but still had the correct check for
       < 0, and no bogus case on the fd for that.  The other bogus cast
       has no effect.
     - I asked someone to fix this, but the result was a larger mess, with
       the type hack in the code (but with the correct spelling of u_int),
       and a verbose comment describing the hack.
     - I eventually fixed this in 2004.  The log message says doesn't have
       as much detail as this mail.  It only says "Don't manually optimize
       for 20 year old compilers ...".
     Now it uses the type hack again.  The 20 year old compilers are 28
     years old now.

FreeBSD-current-April-10-2012 kern_descrip.c:
     7 instances of u_int
     0 of the 7 for this type hack
     3 instances where `unsigned' is misspelled as itself
     3 of the 3 for this type hack
     1 of the 3 misformatted with a space after the cast
     2 instances where `unsigned' is misspelled as `unsigned int'
     0 of the 2 for this type hack
     The above 3 instances where an fd is compared with '< 0' instead of
     using the type hack.

FreeBSD-current kern_descrip.c:
     7 instances of u_int
     0 of the 7 for this type hack
     3 instances where `unsigned' is misspelled as itself
     3 of the 3 for this type hack
     1 of the 3 misformatted with a space after the cast
     6 instances where `unsigned' is misspelled as `unsigned int'
     4 of the 6 for this type hack (in one of these, the cast is redundant
     since the rvalue already has type u_int)
     No instances where an fd is compared with '< 0' in preference to using
     the type hack.

In some places, the hack is done in a much worse way, by type punning
the syscall arg.  E.g., in Lite2 and FreeBSD-3, at least dup() and dup2()
are misdeclared as taking u_int filedescriptor args instead of the actual
int args.  The code could depend on this, but actually duplicates the
type pun by copying the args to local variables with the bogus u_int type.
Then it depends on the type hack (but written without the cast) for
checking the lower limit.

select() was similarly obfuscated.  In Lite2, its nd arg is misdeclared
as u_int in syscalls.master, and was copied to a u_int variable which
was explicitly compared only against the upper limit.  This is fixed
in FreeBSD-3.  The type hack doesn't even work for the relaxed check
on the upper limit in FreeBSD-3.

Bruce


More information about the svn-src-all mailing list