This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
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
(EMFILE).
>
> 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.
[1] https://sourceware.org/ml/libc-alpha/2016-09/msg00006.html