Re: [PATCH v3 4/4] posix: Add posix_spawn_file_actions_closefrom

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella via Libc-alpha:

> This patch adds a way to close a range of file descriptors on
> posix_spawn as a new file action.  The API is similar to the one
> provided by Solaris 11 [1], where the file action causes the all open
> file descriptors greater than or equal to input on to be closed when
> the new process is spawned.

Commit subject should say posix_spawn_file_actions_closefrom_np.

> The function The function posix_spawn_file_actions_closefrom_np is safe
> to be implemented by interacting over /proc/self/fd, since the Linux
> spawni.c helper process does not use CLONE_FILES, so its has own file
> descriptor table and any failure (in /proc operation) aborts the process
> creation and returns an error to the caller.

Duplicate “The function”, ”iterating” instead of “interacting”.

>
> diff --git a/NEWS b/NEWS
> index e6446f0548..60a13edcbe 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -34,6 +34,11 @@ Major new features:
>    greater than given integer.  This function is a GNU extension, although it
>    also present in other systems.
>  
> +* The posix_spawn_file_actions_closefrom_np has been added, enabling
> +  posix_spawn and posix_spawnp to close all file descriptors greater than
> +  a giver integer.  This function is a GNU extension, although Solaris also
> +  provides a similar function.

Missing “function”.  “great than or equal to”.

> diff --git a/posix/Versions b/posix/Versions
> index 7d06a6d0c0..95d7b01126 100644
> --- a/posix/Versions
> +++ b/posix/Versions
> @@ -147,6 +147,9 @@ libc {
>    }
>    GLIBC_2.30 {
>    }
> +  GLIBC_2.33 {
> +    posix_spawn_file_actions_addclosefrom_np;
> +  }

Needs to be GLIBC_2.34 now.  Copyright years need adjusting as well.

> diff --git a/posix/spawn.h b/posix/spawn.h
> index be6bd591a3..e81b4a4e90 100644
> --- a/posix/spawn.h
> +++ b/posix/spawn.h
> @@ -213,6 +213,14 @@ extern int posix_spawn_file_actions_addchdir_np (posix_spawn_file_actions_t *
>  extern int posix_spawn_file_actions_addfchdir_np (posix_spawn_file_actions_t *,
>  						  int __fd)
>       __THROW __nonnull ((1));
> +
> +/* Add an action to close all file descriptor greater than FROM during
> +   spawn.  This affects the subsequent file actions.  */
> +extern int
> +posix_spawn_file_actions_addclosefrom_np (posix_spawn_file_actions_t *,
> +					  int __from)
> +     __THROW __nonnull ((1));
> +
>  #endif

Likewise, “greater than or equal to”.

> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
> new file mode 100644
> index 0000000000..a20e9c9ac7
> --- /dev/null
> +++ b/posix/tst-spawn5.c
> @@ -0,0 +1,283 @@

> +/* Nonzero if the program gets called via `exec'.  */
> +static int restart;

“was called”?

> +/* Hold the four initial argument used to respawn the process.  */
> +static char *initial_argv[7];

four != 7?

> +/* Called on process re-execution.  The arguments are the expected opened
> +   file descriptors.  */
> +_Noreturn static void
> +handle_restart (int argc, char *argv[])
> +{

> +      char *endptr;
> +      long int fd = strtol (e->d_name, &endptr, 10);
> +      if (*endptr != '\0' || fd < 0 || fd > INT_MAX)
> +        FAIL_EXIT1 ("readdir: invalid file descriptor name: /proc/self/fd/%s",
> +                    e->d_name);

Maybe use strtoul here?

> +static void
> +do_test_closefrom (void)
> +{

> +  /* Close a range and add some file actions.  */
> +  {
> +    posix_spawn_file_actions_t fa;
> +    TEST_COMPARE (posix_spawn_file_actions_init (&fa), 0);
> +
> +    TEST_COMPARE (posix_spawn_file_actions_addclosefrom_np (&fa, lowfd + 1), 0);
> +    TEST_COMPARE (posix_spawn_file_actions_addopen (&fa, lowfd, "/dev/null",
> +						    0666, O_RDONLY), 0);
> +    TEST_COMPARE (posix_spawn_file_actions_adddup2 (&fa, lowfd, lowfd + 1), 0);
> +    TEST_COMPARE (posix_spawn_file_actions_addopen (&fa, lowfd, "/dev/null",
> +						    0666, O_RDONLY), 0);
> +
> +    spawn_closefrom_test (&fa, lowfd, lowfd, (int[]){lowfd, lowfd + 1}, 2);

I believe the closefrom should start add lowfd here, to check that the
closing is ordered with regards to the other operations.

> diff --git a/sysdeps/generic/spawn_int_abi.h b/sysdeps/generic/spawn_int_abi.h
> new file mode 100644
> index 0000000000..bfc8961598
> --- /dev/null
> +++ b/sysdeps/generic/spawn_int_abi.h
> @@ -0,0 +1,24 @@
> +/* Internal ABI specific for posix_spawn functionality.  Generic version.

> +#ifndef _SPAWN_INT_ABI_H
> +#define _SPAWN_INT_ABI_H
> +
> +#define __SPAWN_SUPPORT_CLOSEFROM 0
> +
> +#endif /* _SPAWN_INT_H */

Final comment is wrong.  The header is misnamed because the macro does
not actually impact ABI.

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index f157bfffd2..ea87d8b5e6 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c

>  /* The Linux implementation of posix_spawn{p} uses the clone syscall directly
>     with CLONE_VM and CLONE_VFORK flags and an allocated stack.  The new stack
> @@ -280,6 +274,14 @@ __spawni_child (void *arguments)
>  	      if (__fchdir (action->action.fchdir_action.fd) != 0)
>  		goto fail;
>  	      break;
> +
> +	    case spawn_do_closefrom:
> +	      {
> +		int lowfd = action->action.closefrom_action.from;
> +	        int r = INLINE_SYSCALL_CALL (close_range, lowfd, ~0U, 0);
> +		if (r != 0 && !__closefrom_fallback (lowfd))
> +		  goto fail;
> +	      } break;

If you implement the agressive up-to-INT_MAX closing in
__closefrom_fallback, this probably needs a flag that it disables it
when called from here.

Thanks,
Florian