From owner-svn-src-head@FreeBSD.ORG Tue Jun 12 02:53:54 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 402F3106566B; Tue, 12 Jun 2012 02:53:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id B64258FC1B; Tue, 12 Jun 2012 02:53:53 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q5C2rmUN026543 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 12 Jun 2012 12:53:51 +1000 Date: Tue, 12 Jun 2012 12:53:47 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pawel Jakub Dawidek In-Reply-To: <201206112017.q5BKHKsW007722@svn.freebsd.org> Message-ID: <20120612114254.V1572@besplex.bde.org> References: <201206112017.q5BKHKsW007722@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r236917 - head/sys/kern X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jun 2012 02:53:54 -0000 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