This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 1/2] posix: Use posix_spawn on popen
On 29/11/2018 14:41, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>>> The issue is adddup2 action will dup2 the pipe child end to child_pipe_fd
>>>> (in this case '1' since it has closed stdout), so adding another action to
>>>> close is wrong.
>>>>
>>>> [1] https://bugzilla.redhat.com/show_bug.cgi?id=248281
>>>
>>> Okay, thanks for providing the reference.
>>>
>>> It think the fix is incorrect in the sense that POSIX is wrong here. As
>>> specified, popen can launch the new subprocess with a close standard
>>> input/output/error if it came from popen in the parent process, which is
>>> wrong. I believe the actual check should be against fd <= 2, not fd !=
>>> child_std_end. But this is a separate discussion, not related to this
>>> patch.
>>
>> Do you mean our interpretation of POSIX or a POSIX defect?
>
> It's a defect. POSIX should never mandate to close the standard I/O
> streams.
The rationale I can think of is to avoid leak file descriptors to new
popen processes since close-on-exec is not specified in standard.
>
>>> But looking at this, I realized that we have a race condition due to the
>>> use of proc_file_chain_lock: A concurrent call to popen may add a
>>> descriptor to the chain after the list traversal and before the
>>> posix_spawn call. Or the descriptor might now be something else because
>>> pclose was called concurrently. The old code did not have this
>>> particular problem because after the fork, the set of open descriptors
>>> was stable.
>>>
>>> Do you thin kit would be acceptable to extend the scope of the critical
>>> section to include the posix_spawn call? This obviously reduces
>>> concurrency somewhat. I don't think there is a lock ordering issue,
>>> though.
>
>> You are right, the concurrent transverse and update of proc_file_chain
>> lead to a race condition. However, I also think it would be a gain to
>> extend the scope of critical section to include posix_spaw. At least for
>> Linux posix_spawn is noticeable faster and scalable than fork plus execve,
>> specially with large resident memory sets.
>
> The only blocking step (on things like network file systems) is the
> execve in the child process because we do not open any new files. Are
> we okay with that?
IMHO, current posix_spawn should be a gain over fork (also considering vfork
not being an option). Even if add the blocking step over the execve.
>
>> +static bool
>> +spawn_proc (posix_spawn_file_actions_t *fa, FILE *fp, const char *command,
>> + int do_cloexec, int pipe_fds[2], int parent_end, int child_end,
>> + int child_pipe_fd)
>> +{
>> + for (struct _IO_proc_file *p = proc_file_chain; p; p = p->next)
>> + {
>> + int fd = _IO_fileno ((FILE *) p);
>> +
>> + /* If any stream from previous popen() calls has fileno
>> + child_pipe_fd, it has been already closed by the adddup2 action
>> + above. */
>> + if (fd != child_pipe_fd
>> + && __posix_spawn_file_actions_addclose (fa, fd) != 0)
>> + return false;
>> + }
>> +
>> + if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, fa, 0,
>> + (char *const[]){ (char*) "sh", (char*) "-c",
>> + (char *) command, NULL }, __environ) != 0)
>> + return false;
>
> Please add a comment somewhere that the caller has to acquire the chain
> lock, and why that lock needs to cover the posix_spawn call.
>
> (spawn_proc should perhaps be spawn_process, to avoid the
> process/procedure confusion.)
Ack, I changed it to spawn_process and added the comment:
/* POSIX states popen shall ensure that any streams from previous popen()
calls that remain open in the parent process should be closed in the new
child process.
To avoid a race-condition between checking which file descriptors need to
be close (by transversing the proc_file_chain list) and the insertion of a
new one after a successful posix_spawn this function should be called
with proc_file_chain_lock acquired. */
>
> Otherwise, it looks ready to commit to me.
>
> Thanks,
> Florian
>