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

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 31/08/2016 18:13, Rasmus Villemoes wrote:
>> +	  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

I haven't joined the original discussion also (if any) for old implementation
behaviour, but I think this conforms to austin group issue #370 [1] where it
changed:

fails for any of the reasons that would cause close( ), dup2( ), or open( ) to
fail, an error value shall be returned

to:

fails for any of the reasons that would cause close( ), dup2( ), or open( ) to
fail, other than attempting a close( ) on a file descriptor that is in range
but already closed, an error value shall be returned

The documentation at [2] seems to not have this updates issues description.

[1] http://austingroupbugs.net/view.php?id=370
[2] http://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn.html

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

I think this should be an issue iff the program tries call posix_spawn 
with a total number of file descriptors equal to RLIMIT_NOFILE.

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

Right, this should more problematic to handle.  I think an option is to
add a 'fcntl(fd, F_GETFD) != -1 || errno != EBADF' check before the open
syscall and close the file descriptor in this case.