Re: [PATCH 2/2] posix: Clear close-on-exec for posix_spawn adddup2 (BZ#23640)

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 31/12/2018 14:34, Siddhesh Poyarekar wrote:
> My mail client ate the original submission, so responding to your ping.
> 
> On 13/12/18 2:54 AM, Adhemerval Zanella wrote:
>> Ping (x2).
>>
>> On 29/10/2018 16:32, Adhemerval Zanella wrote:
>>> Ping.
>>>
>>> On 19/09/2018 19:46, adhemerval.zanella@linaro.org wrote:
>>>> From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>>>
>>>> Austin Group issue #411 [1] proposes that posix_spawn file action
>>>> posix_spawn_file_actions_adddup2 resets the close-on-exec when
>>>> source and destination refer to same file descriptor.
>>>>
>>>> It solves the issue on multi-thread applications which uses
>>>> close-on-exec as default, and want to hand-chose specifically
>>>> file descriptor to purposefully inherited into a child process.
>>>> Current approach to achieve this scenario is to use two adddup2 file
>>>> actions and a temporary file description which do not conflict with
>>>> any other, coupled with a close file action to avoid leaking the
>>>> temporary file descriptor.  This approach, besides being complex,
>>>> may fail with EMFILE/ENFILE file descriptor exaustion.
>>>>
>>>> This can be more easily accomplished with an in-place removal of
>>>> FD_CLOEXEC.  Although the resulting adddup2 semantic is slight
>>>> different than dup2 (equal file descriptors should be handled as
>>>> no-op), the proposed possible solution are either more complex
>>>> (fcntl action which a limited set of operations) or results in
>>>> unrequired operations (dup3 which also returns EINVAL for same
>>>> file descriptor).
>>>>
>>>> Checked on aarch64-linux-gnu.
>>>>
> 
> BZ # on ChangeLog.

Fixed.

>>>> diff --git a/sysdeps/posix/spawni.c b/sysdeps/posix/spawni.c
>>>> index b138ab4393..d322db5c19 100644
>>>> --- a/sysdeps/posix/spawni.c
>>>> +++ b/sysdeps/posix/spawni.c
>>>> @@ -204,10 +204,25 @@ __spawni_child (void *arguments)
>>>>             break;
>>>>             case spawn_do_dup2:
>>>> -          if (__dup2 (action->action.dup2_action.fd,
>>>> -              action->action.dup2_action.newfd)
>>>> +          /* Austin Group issue #411 requires adddup2 action with source
>>>> +         and destination being equal to remove close-on-exec flag.  */
>>>> +          if (action->action.dup2_action.fd
>>>>             != action->action.dup2_action.newfd)
>>>> -        goto fail;
>>>> +        {
>>>> +          if (__dup2 (action->action.dup2_action.fd,
>>>> +                 action->action.dup2_action.newfd)
>>>> +              != action->action.dup2_action.newfd)
>>>> +            goto fail;
>>>> +        }
>>>> +          else
>>>> +        {
>>>> +          int fd = action->action.dup2_action.newfd;
>>>> +          int flags = __fcntl (fd, F_GETFD, 0);
>>>> +          if (flags == -1)
>>>> +            goto fail;
>>>> +          if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>>> +            goto fail;
>>>> +        }
> 
> It might be simpler to write this as follows to flatten out the nesting a bit.  It also makes the location of the comment a lot more relevant because the clearing of the flag happens right under it:

I followed your suggestion, thanks.

> 
>     if (action->action.dup2_action.fd ==
>         action->action.dup2_action.newfd)
>       {
>     int fd = action->action.dup2_action.newfd;
>     int flags = __fcntl (fd, F_GETFD, 0);
>     if (flags == -1)
>       goto fail;
>     if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>       goto fail;
>       }
>     else if (__dup2 (action->action.dup2_action.fd,
>              action->action.dup2_action.newfd)
>          != action->action.dup2_action.newfd)
>       goto fail;
> 
>>>>             break;
>>>>           }
>>>>       }
>>>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>>>> index 85239cedbf..b2304ffe60 100644
>>>> --- a/sysdeps/unix/sysv/linux/spawni.c
>>>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>>>> @@ -253,10 +253,25 @@ __spawni_child (void *arguments)
>>>>             break;
>>>>             case spawn_do_dup2:
>>>> -          if (__dup2 (action->action.dup2_action.fd,
>>>> -              action->action.dup2_action.newfd)
>>>> +          /* Austin Group issue #411 requires adddup2 action with source
>>>> +         and destination being equal to remove close-on-exec flag.  */
>>>> +          if (action->action.dup2_action.fd
>>>>             != action->action.dup2_action.newfd)
>>>> -        goto fail;
>>>> +        {
>>>> +          if (__dup2 (action->action.dup2_action.fd,
>>>> +                 action->action.dup2_action.newfd)
>>>> +              != action->action.dup2_action.newfd)
>>>> +            goto fail;
>>>> +        }
>>>> +          else
>>>> +        {
>>>> +          int fd = action->action.dup2_action.newfd;
>>>> +          int flags = __fcntl (fd, F_GETFD, 0);
>>>> +          if (flags == -1)
>>>> +            goto fail;
>>>> +          if (__fcntl (fd, F_SETFD, flags & ~FD_CLOEXEC) == -1)
>>>> +            goto fail;
>>>> +        }
>>>>             break;
>>>>           }
>>>>       }
>>>>
> 
> Likewise.
> 
> Siddhesh

Pushed as 805334b26c.