Re: [PATCH v2 2/3] posix: Use posix_spawn on popen

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> Changed from previous version:
>
>   - Added note about BZ#17490 fix.
>   - Use proc_file_chain_lock to access proc_file_chain (BZ#22834).
>
> ---
>
> This patch uses posix_spawn on popen instead of fork and execl.  On Linux
> this has the advantage of much lower memory consumption (usually 32 Kb
> minimum for the mmap stack area).

Does this need a NEWS entry, noting the fork handler aspect?

> +  int op;

The variable name isn't helpful.  It's overloaded for pipe index and
descriptor index in the child.  Maybe use three variables with
appropriate names instead?

> +  {
> +    posix_spawn_file_actions_t fa;
> +    /* posix_spawn_file_actions_init does not fail.  */
> +    __posix_spawn_file_actions_init (&fa);
>  
> -	  /* If any stream from previous popen() calls has fileno
> -	     child_std_end, it has been already closed by the dup2 syscall
> -	     above.  */
> -	  if (fd != child_std_end)
> -	    __close_nocancel (fd);
> -	}
> +    /* The descriptor is already in the one the child will use.  In this case
> +       it must be moved to another one, otherwise there is no safe way to
> +       remove the close-on-exec flag in the child without creating a FD leak
> +       race in the parent.  */

I think “in the one” is a bit confusing here.

> +    if (pipe_fds[1 - op] == 1 - op)
> +      {
> +	int tmp = __fcntl (1 - op, F_DUPFD_CLOEXEC, 0);
> +	if (tmp < 0)
> +	  goto spawn_failure;
> +	__close_nocancel (pipe_fds[1 - op]);

Missing close of pipe_fds[op] in the error case; spawn_failure only
closes the parent end.

> +    if (__posix_spawn_file_actions_adddup2 (&fa, pipe_fds[1 - op], 1 - op)
> +	!= 0)
> +      goto spawn_failure;

Likewise, missing close of parent end.

> +    /* POSIX.2: "popen() shall ensure that any streams from previous popen()
> +       calls that remain open in the parent process are closed in the new
> +       child process." */
> +#ifdef _IO_MTSAFE_IO
> +     _IO_cleanup_region_start_noarg (unlock);
> +     _IO_lock_lock (proc_file_chain_lock);
> +#endif

You can assume that _IO_MTSAFE_IO is always defined here and not carry
over the preprocessor conditionals.

> +    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_send, it has been already closed by the dup2 syscall
> +	   above.  */
> +	if (fd != 1 - op
> +	    && __posix_spawn_file_actions_addclose (&fa, fd) != 0)
> +	  goto spawn_failure;
> +      }

The jump out of cleanup region is undefined, I think.  Missing close of
parent end.

> +#ifdef _IO_MTSAFE_IO
> +      _IO_lock_unlock (proc_file_chain_lock);
> +      _IO_cleanup_region_end (0);
> +#endif
> +
> +    if (__posix_spawn (&((_IO_proc_file *) fp)->pid, _PATH_BSHELL, &fa, 0,
> +		     (char *const[]){ (char*) "sh", (char*) "-c",
> +		     (char *) command, NULL }, __environ) != 0)
> +      {
> +      spawn_failure:
> +	__posix_spawn_file_actions_destroy (&fa);
> +	__close_nocancel (pipe_fds[1 - op]);
> +	__set_errno (ENOMEM);
> +	return NULL;

Not sure what's supposd to happen to the parent end in this case and if
you need to close it here.

> +      }
> +
> +    __posix_spawn_file_actions_destroy (&fa);
> +  }
> +  __close_nocancel (pipe_fds[1 - op]);
> +  if (((_IO_proc_file *) fp)->pid < 0)
>      {
> -      __close_nocancel (parent_end);
> +      __close_nocancel (pipe_fds[op]);
>        return NULL;
>      }

How can ->pid be negative without a posix_spawn failure?

Thanks,
Florian