Date: Mon, 25 Nov 2002 21:32:05 +1100 (EST) From: Bruce Evans <bde@zeta.org.au> To: Olivier Houchard <cognet@ci0.org> Cc: freebsd-audit@FreeBSD.ORG Subject: Re: do_dup patch Message-ID: <20021125210116.I56453-100000@gamplex.bde.org> In-Reply-To: <20021125093228.GA10213@ci0.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
<sys/sysproto.h> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20021125210116.I56453-100000>