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

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