Re: [PATCH v3 1/2] posix: Use posix_spawn on popen

[Florian Weimer <fweimer at redhat dot com>]

* 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.

>> 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?

> +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.)

Otherwise, it looks ready to commit to me.

Thanks,
Florian