Re: [PATCH] posix: Add posix_spawn_file_actions_closefrom

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> +int
> +__posix_spawn_file_actions_addclosefrom (posix_spawn_file_actions_t
> +					 *file_actions, int from)
> +{
> +#if __SUPPORT_SPAWN_CLOSEFROM
> +  struct __spawn_action *rec;
> +
> +  if (!__spawn_valid_fd (from))
> +    return EBADF;

I don't think this check (against _SC_OPEN_MAX) is valid.  >= 0 would be

> diff --git a/posix/spawn_int_abi.h b/posix/spawn_int_abi.h
> new file mode 100644
> index 0000000000..3008937e21

> +/* The closefrom file actions requires either a syscall or an arch-specific
> +   way to interact over all file descriptors and act uppon them (such
> +   /proc/self/fd on Linux).  */
> +#define __SUPPORT_SPAWN_CLOSEFROM 0

I think __SUPPORT should be reserved support/, so please move SPAWN to
the front.

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index c1abf3f960..4e35dc21e7 100644
> --- a/sysdeps/unix/sysv/linux/spawni.c
> +++ b/sysdeps/unix/sysv/linux/spawni.c

> +/* Close all file descriptor up to FROM by interacting /proc/self/fd.
> +   Any failure should */
> +static bool
> +spawn_closefrom (int from)

Truncated comment.

> +{
> +  /* Each name in /proc/self/fd will be an integer up to sizeof (int),
> +     so d_name will be most 'sizeof (int) * 3 + 1'.  This makes each
> +     'dirent struct' entry to hold up to
> +     'offsetof (struct libc_dirent_2, d_name)' plus the d_name size.  */
> +
> +  enum
> +  {
> +    dirent_base_size  = offsetof (struct dirent64, d_name),
> +    d_name_max_length = sizeof (int) * 3 + 1,
> +    dirent_max_size   = ALIGN_UP (dirent_base_size + d_name_max_length,
> +				  sizeof (long double))
> +  };
> +
> +  /* Increasing the buffer size incurs in less getdents syscalls from
> +     readdir, however it would require more stack size to be allocated
> +     on __spawnix.  */
> +  char buffer[sizeof (DIR) + 16 * dirent_max_size];
> +
> +  DIR *dp;
> +  if ((dp = __opendir_inplace ("/proc/self/fd", buffer, sizeof buffer))
> +      == NULL)
> +    return false;

Maybe you could have a look at my direntries patch and see if it would
be useful here?

> +  bool ret = true;
> +  struct dirent64 *dirp;
> +  while ((dirp = __readdir64 (dp)) != NULL)
> +    {
> +      if (dirp->d_name[0] == '.')
> +        continue;
> +
> +      char *endptr;
> +      long int fd = strtol (dirp->d_name, &endptr, 10);
> +      if (*endptr != '\0' || fd < 0 || fd > INT_MAX)
> +	{
> +	  ret = false;
> +	  break;
> +	}
> +
> +      if (fd == dirfd (dp) || fd < from)
> +        continue;
> +
> +      __close (fd);

I still think this should rewind (seek to zero) if any file descriptors
have been closed.

With the direntries iterator, you could process everything that has been
read by the last getdents64 call.  With readdir64, the buffer boundary
is implicit.

> +  /* The __spawni_child uses about 768 on x86_64 (including the stack for
> +     opendir_inplace), so add some extra space.  */
> +  const size_t stack_slack = 1024;
> +
>    /* Add a slack area for child's stack.  */
> -  size_t argv_size = (argc * sizeof (void *)) + 512;
> +  size_t argv_size = (argc * sizeof (void *)) + stack_slack;
>    /* We need at least a few pages in case the compiler's stack checking is
>       enabled.  In some configs, it is known to use at least 24KiB.  We use
>       32KiB to be "safe" from anything the compiler might do.  Besides, the

I think this should be aligned witht the computation above.
Alternatively, you could allocate the buffer on the original stack.

Thanks,
Florian