From owner-freebsd-audit Mon Nov 25 3:12:52 2002 Delivered-To: freebsd-audit@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0525B37B401 for ; Mon, 25 Nov 2002 03:12:49 -0800 (PST) Received: from cognet.ci0.org (cognet.ci0.org [80.65.224.102]) by mx1.FreeBSD.org (Postfix) with ESMTP id AE43943E4A for ; Mon, 25 Nov 2002 03:12:47 -0800 (PST) (envelope-from mlfbsd@cognet.ci0.org) Received: from cognet.ci0.org (cognet.ci0.org [80.65.224.102] (may be forged)) by cognet.ci0.org (8.12.6/8.12.6) with ESMTP id gAPB93Fn011233; Mon, 25 Nov 2002 12:09:03 +0100 (CET) (envelope-from mlfbsd@cognet.ci0.org) Received: (from mlfbsd@localhost) by cognet.ci0.org (8.12.6/8.12.6/Submit) id gAPB92aM011232; Mon, 25 Nov 2002 12:09:02 +0100 (CET) Date: Mon, 25 Nov 2002 12:09:02 +0100 From: Olivier Houchard To: Bruce Evans Cc: freebsd-audit@FreeBSD.ORG Subject: Re: do_dup patch Message-ID: <20021125110902.GA10961@ci0.org> References: <20021125093228.GA10213@ci0.org> <20021125210116.I56453-100000@gamplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20021125210116.I56453-100000@gamplex.bde.org> User-Agent: Mutt/1.5.1i Sender: owner-freebsd-audit@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On Mon, Nov 25, 2002 at 09:32:05PM +1100, Bruce Evans wrote: > On Mon, 25 Nov 2002, Olivier Houchard wrote: > > > This patch makes the "new" and "old" do_dup arguments unsigned int instead of > > int. I can't see any reason they would have to be int, and right now a call to > > dup or dup2 with an invalid negative fd, such as dup(-999999); will panic a > > -CURRENT box (the problem doesn't exist on -STABLE because the file descriptor > > validity is checked in dup() and dup2()). > > Is there anything wrong with committing this ? > > Yes; it is sort of backwards. > > File descriptors are small positive (signed) ints, and the dup() and dup2() > syscalls take file descriptors as parameters. However, dup() and dup2() > are misdeclared as taking u_int's as their parameters. This used to be > obfuscate/optimize away an explicit comparison with 0 in the bounds check > for the parameters. E.g., in RELENG_4: > > % #ifndef _SYS_SYSPROTO_H_ > % struct dup2_args { > % u_int from; > % u_int to; > ^^^^^ > Bogus type for bogus optimization. This is pseudo-code; see > for the actual declaration. This misdeclaration > needs something like normal 2's complement machines to work since > we never actually convert the parameters from ints to u_ints; > we just pass ints and pretend that they are u_ints. > > % }; > % #endif > % /* ARGSUSED */ > % int > % dup2(p, uap) > % struct proc *p; > % struct dup2_args *uap; > % { > % register struct filedesc *fdp = p->p_fd; > % register u_int old = uap->from, new = uap->to; > % int i, error; > % > % retry: > % if (old >= fdp->fd_nfiles || > ^^^^^^^^^^^^^^^^^^^^^ > > Bogus optimization. The upper and lower bounds are checked in a > single comparison. The only reason to do this seems to be to > hand-optimize for 20+-year old compilers on pdp11-like machines > where the 2 comparisons can be collapes into a single machine > instruction and the compiler doesn't do this automatically. > > % fdp->fd_ofiles[old] == NULL || > % new >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur || > % new >= maxfilesperproc) { > % return (EBADF); > % } > > In -current, this has rotted to: > > % int > % dup2(td, uap) > % struct thread *td; > % struct dup2_args *uap; > % { > % > % return (do_dup(td, DUP_FIXED, (int)uap->from, (int)uap->to, > ^^^^^ ^^^^^ > > Bogus casts to break warnings about our type errors. > > % td->td_retval)); > > % } > % ... > % static int > % do_dup(td, type, old, new, retval) > % enum dup_type type; > % int old, new; > % register_t *retval; > % struct thread *td; > % { > % register struct filedesc *fdp; > % struct proc *p; > % struct file *fp; > % struct file *delfp; > % int error, newfd; > % > % p = td->td_proc; > % fdp = p->p_fd; > % > % /* > % * Verify we have a valid descriptor to dup from and possibly to > % * dup to. > % */ > % FILEDESC_LOCK(fdp); > % if (old >= fdp->fd_nfiles || fdp->fd_ofiles[old] == NULL || > ^^^^^^^^^^^^^^^^^^^^^ > > Bug. The bogus optimization has been broken by changing the types. > > % new >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur || > % new >= maxfilesperproc) { > % FILEDESC_UNLOCK(fdp); > % return (EBADF); > % } > > Bruce Right. So what would be the best ? Changing dup and dup2 prototypes in syscall.master to make new and old signed instead of unsigned and checking that new >= 0 && old >= 0 in do_dup ? Thanks, Olivier To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message