From owner-freebsd-audit Mon Nov 25 2:19: 2 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 0E0A237B401 for ; Mon, 25 Nov 2002 02:19:00 -0800 (PST) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id DDDE043E4A for ; Mon, 25 Nov 2002 02:18:58 -0800 (PST) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id VAA32197; Mon, 25 Nov 2002 21:18:41 +1100 Date: Mon, 25 Nov 2002 21:32:05 +1100 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Olivier Houchard Cc: freebsd-audit@FreeBSD.ORG Subject: Re: do_dup patch In-Reply-To: <20021125093228.GA10213@ci0.org> Message-ID: <20021125210116.I56453-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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, 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 To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-audit" in the body of the message