cvs commit: src/sys/kern kern_descrip.c

[Bruce Evans]

On Thu, 29 May 2008, Ed Schouten wrote:

> * Robert Watson <rwatson at FreeBSD.org> wrote:
>> This sounds like a good candidate for a regression test -- do we have a
>> dup/dup2/F_DUPFD/F_DUP2FD test?  If not, perhaps we should, in light of
>> the opportunity for further bugs and regressions.
>
> It looks like we already have regression tests for dup/dup2/etc --
> kern_descrip.c 1.325 mentions them.
>
> I saw FreeBSD also implements F_DUP2FD (which is a non-standard
> extension). Right now this command returns EBADF when you do the
> following:
>
> 	fcntl(0, F_DUP2FD, -1);		// below zero
> 	fcntl(0, F_DUP2FD, 1000000);	// too high
>
> This is exactly the same as what dup2() does, but is inconsistent with
> fcntl() in general. EBADF should be returned if the `fd' argument is
> invalid. It should not apply to the argument.

No, EBADF applies to both the open fd and the new fd for dup2() (since
that is what dup2() did initially and has become Standard).  F_DUP2FD
is apparently supposed to be equivalent to dup2(), so it must be bug-for
bug equivalent.

dup2() was broken in rev.1.219.  It actually returns EMFILE if the new
fd is too high.  It remains unbroken and returns EBADF if the new fd
is below zero, but this gives another inconsistency.  I think F_DUP2FD
inherits these bugs.  F_DUPFD consistently returns EINVAL if the new
fd is either too high or below zero.  More details in previously-sent
private mail.

> We could consider applying the following patch:
>
> --- sys/kern/kern_descrip.c
> +++ sys/kern/kern_descrip.c
> @@ -423,7 +423,8 @@
>
> 	case F_DUP2FD:
> 		tmp = arg;
> -		error = do_dup(td, DUP_FIXED, fd, tmp, td->td_retval);
> +		error = do_dup(td, DUP_FIXED|DUP_FCNTL, fd, tmp,
> +		    td->td_retval);
> 		break;
>
> 	case F_GETFD:

Style bug (missing spaces around binary operator).

> --- lib/libc/sys/fcntl.2
> +++ lib/libc/sys/fcntl.2
> @@ -452,14 +452,6 @@
> The argument
> .Fa cmd
> is
> -.Dv F_DUP2FD ,
> -and
> -.Fa arg
> -is not a valid file descriptor.
> -.Pp
> -The argument
> -.Fa cmd
> -is
> .Dv F_SETLK
> or
> .Dv F_SETLKW ,
> @@ -502,6 +494,8 @@
> argument
> is
> .Dv F_DUPFD
> +or
> +.Dv F_DUP2FD
> and
> .Fa arg
> is negative or greater than the maximum allowable number
>
> Any comments?

I think F_DUP2FD and its man page are as intended and don't need changing.
But dup2()'s man page needs changes to spell out the details of the
differences between the error handling of dup() and dup2() (it is shared
with dup()'s man page but no differences are mentioned).  Comparing with
POSIX shows that the main differences are:
- [EMFILE] doesn't apply to dup2(), since the new fd is available unless
   it is out of bounds, and the error for out of bounds is EBADF
- for dup2(), it is not an error for the new fd to be "inactive",

and that the main bugs which aren't differences are:

- "active" is a bad way of saying "open"
- "valid" is a fuzzy way of saying "not (negative or >= {OPEN_MAX})".
   fcntl.2 already says this better (see above quote; in the unquoted
   part it spells {OPEN_MAX} as "see getdtablesize(2)").  I think
   FreeBSD man pages should explicitly refer to intro(2) for the the
   details of this, but intro(2) is about 20 years out of date here --
   it says that releases have a fixed limit of 64 and refers to
   getdtablesize(2) and never mentions {OPEN_MAX}.
- [EINTR] is not described.  I'm not sure if it applies to the dup()
   family when syscalls are restarted, but it applies when they are not
   and is mentioned for open(2).

Bruce