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