cvs commit: src/sys/kern kern_descrip.c

[Antoine Brodin]

On Thu, May 29, 2008 at 10:24 AM, Ed Schouten <ed at 80386.nl> wrote:
> * Robert Watson <rwatson at FreeBSD.org> wrote:
>>
>> On Wed, 28 May 2008, Ed Schouten wrote:
>>
>>>  Remove redundant checks from fcntl()'s F_DUPFD.
>>>
>>>  Right now we perform some of the checks inside the fcntl()'s F_DUPFD
>>>  operation twice. We first validate the `fd' argument. When finished,
>>>  we validate the `arg' argument. These checks are also performed inside
>>>  do_dup().
>>>
>>>  The reason we need to do this, is because fcntl() should return different
>>>  errno's when the `arg' argument is out of bounds (EINVAL instead of
>>>  EBADF). To prevent the redundant locking of the PROC_LOCK and
>>>  FILEDESC_SLOCK, patch do_dup() to support the error semantics required
>>>  by fcntl().
>>
>> 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.
>
> 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:
> --- 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?

Hello Ed,

On Solaris, dup2 and fcntl(F_DUP2FD) are totally equivalent (they
return the same errno).
I have no strong feelings about this.

Cheers,

Antoine