[PATCH 3/3] posix: Add posix_spawn_file_actions_closefrom

[Adhemerval Zanella]

On 22/12/2020 08:32, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> +* 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 interger.  This function is a GNU extension, although Solaris also
>> +  provides a similar function.
> 
> Two typos: “giver interger”

Ack.

> 
>> diff --git a/posix/spawn.h b/posix/spawn.h
>> index be6bd591a3..21ea563425 100644
>> --- a/posix/spawn.h
>> +++ b/posix/spawn.h
>> @@ -213,6 +213,13 @@ 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
> 
> Line length issue (but I'm not sure how to avoid that).

Maybe move the argument to next line aligning with __THROW?

extern int posix_spawn_file_actions_addclosefrom_np (
     posix_spawn_file_actions_t *, int __from)
     __THROW __nonnull ((1));

?

> 
>> diff --git a/posix/spawn_faction_addclosefrom.c b/posix/spawn_faction_addclosefrom.c
>> new file mode 100644
>> index 0000000000..3a295580bc
>> --- /dev/null
>> +++ b/posix/spawn_faction_addclosefrom.c
> 
>> +int
>> +__posix_spawn_file_actions_addclosefrom (posix_spawn_file_actions_t
>> +					 *file_actions, int from)
>> +{
>> +#if __SPAWN_SUPPORT_CLOSEFROM
>> +  struct __spawn_action *rec;
>> +
>> +  if (!__spawn_valid_fd (from))
>> +    return EBADF;
>> +
>> +  /* Allocate more memory if needed.  */
>> +  if (file_actions->__used == file_actions->__allocated
>> +      && __posix_spawn_file_actions_realloc (file_actions) != 0)
>> +    /* This can only mean we ran out of memory.  */
>> +    return ENOMEM;
>> +
>> +  /* Add the new value.  */
>> +  rec = &file_actions->__actions[file_actions->__used];
>> +  rec->tag = spawn_do_closefrom;
>> +  rec->action.closefrom_action.from = from;
>> +
>> +  /* Account for the new entry.  */
>> +  ++file_actions->__used;
>> +
>> +  return 0;
>> +#else
>> +  __set_errno (EINVAL);
>> +  return -1;
>> +#endif
>> +}
> 
> Should this return EINVAL for the fallback case?

Indeed, this should not set errno.

> 
>> diff --git a/posix/spawn_int_abi.h b/posix/spawn_int_abi.h
>> new file mode 100644
>> index 0000000000..5fda4d3442
>> --- /dev/null
>> +++ b/posix/spawn_int_abi.h
>> @@ -0,0 +1,27 @@
>> +/* Internal ABI specific for posix_spawn functionality.  Generic version.
>> +   Copyright (C) 2020 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   The GNU C Library is free software; you can redistribute it and/or
>> +   modify it under the terms of the GNU Lesser General Public
>> +   License as published by the Free Software Foundation; either
>> +   version 2.1 of the License, or (at your option) any later version.
>> +
>> +   The GNU C Library is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +   Lesser General Public License for more details.
>> +
>> +   You should have received a copy of the GNU Lesser General Public
>> +   License along with the GNU C Library; if not, see
>> +   <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef _SPAWN_INT_ABI_H
>> +#define _SPAWN_INT_ABI_H
>> +
>> +/* 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 __SPAWN_SUPPOR_CLOSEFROM 0
>> +
>> +#endif /* _SPAWN_INT_H */
> 
> I think Hurd has support for this via __getdtablesize, so perhaps this
> is not needed?
> 
> In any case, ABI is a bit of a misnomer here, and this file is
> uncessary, given the sysdeps/generic file.

Yes, I add the generic one to fix a hurd build fail and I forgot to remove
this one.

> 
>> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>> new file mode 100644
>> index 0000000000..1f901d112b
>> --- /dev/null
>> +++ b/posix/tst-spawn5.c
> 
>> +enum
>> +{
>> +  maximum_fd = 100,
>> +  half_fd = maximum_fd / 2,
>> +};
>> +static int fds[maximum_fd + 1];
> 
> The usual comments about file descriptor layout. 8-)

Ack, I will remove the array usage.

> 
>> +static void
>> +do_test_closefrom (void)
>> +{
>> +  for (int i = 0; i < array_length (fds); i++)
>> +    fds[i] = xopen ("/dev/null", O_WRONLY, 0);
> 
> This should perhaps check that the descriptors 57, …, 90 have actually
> been opened, perhaps implicitly by checking that the resulting
> descriptors end up as one block.
> 
>> +  /* Close the remmaining but the last one.  */
> 
> Typo: “remmaining”
> 
>> +  if (restart)
>> +    handle_restart (argc, argv);
>> +
>> +  initial_argv[0] = argv[1]; /* path for ld.so  */
>> +  initial_argv[1] = argv[2]; /* "--library-path"  */
>> +  initial_argv[2] = argv[3]; /* the library path  */
>> +  initial_argv[3] = argv[4]; /* the application name  */
>> +  initial_argv[4] = (char *) "--direct";
>> +  initial_argv[5] = (char *) "--restart";
>> +
>> +  do_test_closefrom ();
>> +
>> +  return 0;
>> +}
>> +
>> +#define TEST_FUNCTION_ARGV do_test
>> +#include <support/test-driver.c>
> 
> You could simplify the reinvocation logic by making this a static or a
> container test.

I prefer to keep this exercises this test on some platform where container
tests are not really supported.

> 
>> diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>> index f157bfffd2..f496578d19 100644
>> --- a/sysdeps/unix/sysv/linux/spawni.c
>> +++ b/sysdeps/unix/sysv/linux/spawni.c
>> @@ -16,22 +16,17 @@
>>     License along with the GNU C Library; if not, see
>>     <https://www.gnu.org/licenses/>.  */
>>  
>> -#include <spawn.h>
>> -#include <fcntl.h>
>> -#include <paths.h>
>> -#include <string.h>
>> -#include <sys/resource.h>
>> -#include <sys/wait.h>
>> -#include <sys/param.h>
>> -#include <sys/mman.h>
>> -#include <not-cancel.h>
>> +#include <arch-fd_to_filename.h>
>> +#include <internal-signals.h>
>> +#include <ldsodefs.h>
>>  #include <local-setxid.h>
>> +#include <not-cancel.h>
>> +#include <paths.h>
>>  #include <shlib-compat.h>
>> -#include <nptl/pthreadP.h>
>> -#include <dl-sysdep.h>
>> -#include <libc-pointer-arith.h>
>> -#include <ldsodefs.h>
>> -#include "spawn_int.h"
>> +#include <spawn.h>
>> +#include <spawn_int.h>
>> +#include <sysdep.h>
>> +#include <sys/resource.h>
> 
> Suprious changes?

In fact I cleanup the include range a bit, if you prefer I can remove this
change from this set.