This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] posix: Fix open file action for posix_spawn on Linux

On 19/09/2016 12:25, Florian Weimer wrote:
> On 09/19/2016 04:39 PM, Adhemerval Zanella wrote:
>> On 19/09/2016 09:53, Florian Weimer wrote:
>>> On 09/19/2016 02:41 PM, Adhemerval Zanella wrote:
>>>> +/* Return if file descriptor is opened.  */
>>>> +static inline int
>>>> +fd_is_valid (int fd)
>>>> +{
>>>> +  return fcntl_not_cancel_2 (fd, F_GETFD) != -1 || errno != EBADF;
>>>> +}
>>>> +
>>>>  /* Function used in the clone call to setup the signals mask, posix_spawn
>>>>     attributes, and file actions.  It run on its own stack (provided by the
>>>>     posix_spawn call).  */
>>>> @@ -219,6 +226,15 @@ __spawni_child (void *arguments)
>>>>          case spawn_do_open:
>>>>            {
>>>> +        /* POSIX states that if fildes was already an open file descriptor,
>>>> +           it shall be closed before the new file is opened.  This avoid
>>>> +           pontential issues when posix_spawn plus addopen action is called
>>>> +           with the process already at maximum number of file descriptor
>>>> +           opened and also for multiple actions on single-open special
>>>> +           paths (like /dev/watchdog).  */
>>>> +        if (fd_is_valid (action->action.open_action.fd))
>>>> +          close_not_cancel (action->action.open_action.fd);
>>> It's not clear to me why you can't just close the file descriptor unconditionally.  It does not seem to matter whether you perform fcntl or close on an invalid file descriptor.
>> Indeed it seems superfluous to check for file descriptor validity before
>> calling close, I will remove it.  With this changes, do you have any
>> pending issues for the patch?
> I'd rather suggest something like this, avoiding the close in the common case when the open call succeeds.

This approach has the issue that to 1. potentially trigger the dup + close
case more often because the new fd won't be equal to the expected one,
while closing the file descriptor first might avoid this issue; and 2.
to always trigger a failure in the behaviour described in the patch

> The sysdeps/posix implementation would need a similar change.

Yes, that's why I state this patch only fixes the Linux part.
> However, the new test case consistently fails for me because of the pipe2 call in the Linux implementation.

Sorry, I should have stated that it requires the pipe removal
patch [1] on posix_spawn.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]