Re: [PATCH] RFC: Add posix_spawn_file_actions_closefrom

[Florian Weimer <fweimer at redhat dot com>]

* Adhemerval Zanella:

> diff --git a/posix/spawn.h b/posix/spawn.h
> index 471dbea022..095ee67a26 100644
> --- a/posix/spawn.h
> +++ b/posix/spawn.h
> @@ -213,6 +213,12 @@ 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 durint
> +   spawn.  This affects the subsequent file actions.  */
> +extern int posix_spawn_file_actions_addclosefrom_np (posix_spawn_file_actions_t *,
> +						     int __from);
> +

Missing __THROW __nonnull ((11)), I think.

> diff --git a/posix/spawn_faction_addclosefrom.c b/posix/spawn_faction_addclosefrom.c
> new file mode 100644
> index 0000000000..24e6c62dbc
> --- /dev/null
> +++ b/posix/spawn_faction_addclosefrom.c

Can we implement this on Hurd fairly quickly?  Otherwise, this should
probable be a stub that returns ENOSYS.

> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
> new file mode 100644
> index 0000000000..7ed03810e4

> +static int
> +do_test (int argc, char *argv[])
> +{
> +  /* We must have one or four parameters left if called initially:
> +       + path for ld.so		optional
> +       + "--library-path"	optional
> +       + the library path	optional
> +       + the application name
> +
> +     Plus one parameter to indicate which test to execute through
> +     re-execution.
> +
> +     So for default usage without --enable-hardcoded-path-in-tests, it
> +     will be called initially with 5 arguments and later with 2.  For
> +     --enable-hardcoded-path-in-tests it will be called with 2 arguments
> +     regardless.  */

You could run this as a container test, so that this would become
unnecessary.  (Or link statically.)

The test doesn't exercise the gaps case.

> diff --git a/sysdeps/posix/opendir.c b/sysdeps/posix/opendir.c
> index 8e3ba480b7..b576b3233d 100644
> --- a/sysdeps/posix/opendir.c
> +++ b/sysdeps/posix/opendir.c

> +DIR *
> +__alloc_dir (int fd, bool close_fd, const struct stat64 *statp,
> +	     void *buffer, size_t size)
>  {
>    /* We have to set the close-on-exit flag if the user provided the
>       file descriptor.  */
> @@ -114,31 +147,44 @@ __alloc_dir (int fd, bool close_fd, int flags, const struct stat64 *statp)
>      allocation = MIN (MAX ((size_t) statp->st_blksize, default_allocation),
>  		      MAX_DIR_BUFFER_SIZE);
>  #endif
> -
> -  DIR *dirp = (DIR *) malloc (sizeof (DIR) + allocation);
> -  if (dirp == NULL)
> +  DIR *dirp;
> +  if (!buffer)

Style issue, I think: buffer != NULL.

Otherwise, this is a fairly nice hack to get an async-signal-safe
readdir.  But I'm not sure that it's what we need here.  See below.

> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
> index c1abf3f960..fe0ff95825 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)
> +{
> +  /* 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 (struct __dirstream) + 2 * sizeof (struct dirent)];

We could allocate this on the heap, in the parent.  Maybe we could
opendir in the parent, and play with the underlying descriptor in the
child?  Then you wouldn't need to add __opendir_inplace at all.  Given
that we know what our implementation looks like, this should be fairly
safe.

> +  DIR *dp;
> +  if ((dp = __opendir_inplace ("/proc/self/fd", buffer, sizeof buffer))
> +      == NULL)
> +    return false;

This could check for ENFILE/EMFILE/ENOMEM and try closing descriptors
directly in case of that error, to make room for the new descriptor.
But perhaps that's not worth the complexity.

> +  bool ret = true;
> +  struct dirent *dirp;
> +  while ((dirp = __readdir (dp)) != NULL)

Should this be __readdir64?

> +    {
> +      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);
> +    }
> +  __closedir (dp);
> +
> +  return ret;
> +}

I'm not sure if this is entirely correct.  If we close some descriptors,
and then readdir calls getdents64, what will the kernel return?  Will
there be a gap in the descriptor list?  (Curiously, it's the same issue
we have the the fork handler list. 8-)

If you share my concerns, maybe we should call getdents64 directly and
parse the buffer?  And restart after the buffer has been exhausted?
(I have a patch with such parser functions and planned to submit it
after the getdents64 syscall wrapper went in.)

> +
>  /* Function used in the clone call to setup the signals mask, posix_spawn
>     attributes, and file actions.  It run on its own stack (provided by the
>     posix_spawn call).  */
> @@ -280,6 +321,11 @@ __spawni_child (void *arguments)
>  	      if (__fchdir (action->action.fchdir_action.fd) != 0)
>  		goto fail;
>  	      break;
> +
> +	    case spawn_do_closefrom:
> +	      if (!spawn_closefrom (action->action.closefrom_action.from))
> +		goto fail;
> +	      break;
>  	    }
>  	}

The Hurd and generic implementations will need to be updated to handle
spawn_do_closefrom as well, otherwise they will fail to build.

Thanks,
Florian