Re: [PATCH v2 3/3] posix: New Linux posix_spawn{p} implementation

[Rasmus Villemoes <rv at rasmusvillemoes dot dk>]

On Mon, Feb 01 2016, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:


> +      for (cnt = 0; cnt < file_actions->__used; ++cnt)
> +	{
> +	  struct __spawn_action *action = &file_actions->__actions[cnt];
> +
> +	  /* Dup the pipe fd onto an unoccupied one to avoid any file
> +	     operation to clobber it.  */
> +	  if ((action->action.close_action.fd == p)
> +	      || (action->action.open_action.fd == p)
> +	      || (action->action.dup2_action.fd == p))
> +	    {
> +	      if ((ret = __dup (p)) < 0)
> +		goto fail;
> +	      p = ret;
> +	    }
> +

Rather late to the party, but I think there's a few bugs here. Most
importantly, dup() doesn't preserve the CLOEXEC flag, so if we do move
the write end around like this, the fd will not automatically be closed
during the exec, and hence the parent won't receive EOF and will block
in read() call until the child finally exits. That's easily fixable with
fcntl(p, F_DUPFD_CLOEXEC, 0). Pretty annoying to add a test for, though.

Then there's the condition. First of all I think it's a little ugly
reading the fd member from the "irrelevant" union members, even if gcc
probably optimizes this to a single comparison. But more importantly,
for the purpose of preventing clobbering our back channel to our parent,
in the case of dup2_action the target aka newfd is much more relevant.

That doesn't mean we don't also have to do some checking of the source
of a dup2 action: If we allow dup2'ing it, we'd end up with the same
problem as above; some copy of the write end of the pipe not being
closed by exec(). Arguably, from the application's POV, p is not an open
file descriptor, so we could fail the action (and hence the entire
spawn) with EBADF. In fact, I can't really see what else we could
do. For the close action, we can get by with just dupping the fd away
and letting the action close the original (effectively making the action
a no-op), but I also think that's an application bug.

So I think the pipe clobber checking should be done in each action
individually, since there's different logic that needs to be applied in
each case.

> +	  switch (action->tag)
> +	    {
> +	    case spawn_do_close:
> +	      if ((ret =
> +		   close_not_cancel (action->action.close_action.fd)) != 0)
> +		{
> +		  if (!have_fdlimit)
> +		    {
> +		      __getrlimit64 (RLIMIT_NOFILE, &fdlimit);
> +		      have_fdlimit = true;
> +		    }
> +
> +		  /* Signal errors only for file descriptors out of range.  */
> +		  if (action->action.close_action.fd < 0
> +		      || action->action.close_action.fd >= fdlimit.rlim_cur)
> +		    goto fail;
> +		}

I may have missed it in the original discussion, but what is the
rationale for this? POSIX says

  If the file_actions argument is not NULL, and specifies any close,
  dup2, or open actions to be performed, and if posix_spawn() or
  posix_spawnp() fails for any of the reasons that would cause close(),
  dup2(), or open() to fail, an error value shall be returned as
  described by close(), dup2(), and open(), respectively
  
> +	      break;
> +
> +	    case spawn_do_open:
> +	      {
> +		ret = open_not_cancel (action->action.open_action.path,
> +				       action->action.
> +				       open_action.oflag | O_LARGEFILE,
> +				       action->action.open_action.mode);
> +
> +		if (ret == -1)
> +		  goto fail;
> +
> +		int new_fd = ret;
> +
> +		/* Make sure the desired file descriptor is used.  */
> +		if (ret != action->action.open_action.fd)
> +		  {
> +		    if ((ret = __dup2 (new_fd, action->action.open_action.fd))
> +			!= action->action.open_action.fd)
> +		      goto fail;
> +
> +		    if ((ret = close_not_cancel (new_fd)) != 0)
> +		      goto fail;
> +		  }
> +	      }

This is also how I'd have implemented it, but POSIX explicitly says

  If fildes was already an open file descriptor, it shall be closed
  before the new file is opened.

Some, if slightly pathological, examples of how the difference could be
observed:

* ENFILE/EMFILE

* Some single-open special device; following POSIX,
  'action_open("/dev/watchdog", 47); action_open("/dev/watchdog", 47);'
  should both succeed if the first does, whereas the second will fail
  with the current code.

> +	      break;
> +
> +	    case spawn_do_dup2:
> +	      if ((ret = __dup2 (action->action.dup2_action.fd,
> +				 action->action.dup2_action.newfd))
> +		  != action->action.dup2_action.newfd)
> +		goto fail;
> +	      break;
> +	    }
> +	}
> +    }
> +
> +  /* Set the initial signal mask of the child if POSIX_SPAWN_SETSIGMASK
> +     is set, otherwise restore the previous one.  */
> +  __sigprocmask (SIG_SETMASK, (attr->__flags & POSIX_SPAWN_SETSIGMASK)
> +		 ? &attr->__ss : &args->oldmask, 0);
> +
> +  args->exec (args->file, args->argv, args->envp);
> +
> +  /* This is compatibility function required to enable posix_spawn run
> +     script without shebang definition for older posix_spawn versions
> +     (2.15).  */
> +  maybe_script_execute (args);
> +
> +  ret = -errno;
> +
> +fail:
> +  /* Since sizeof errno < PIPE_BUF, the write is atomic. */
> +  ret = -ret;
> +  if (ret)
> +    while (write_not_cancel (p, &ret, sizeof ret) < 0)
> +      continue;

This also seems wrong, at least if the intention is to write the proper
errno value back. That only happens if we reach fail: after failing to
exec - in most or all other cases, ret has the value -1, so we'd be
writing EPERM to our parent. So why not just pull the value from errno
in all cases?

Rasmus