Re: [PATCH v8 5/7] posix: Add pidfd_spawn and pidfd_spawnp (BZ 30349)

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> Returning a pidfd allows a process to keep a race-free handle for a
> child process, otherwise, the caller will need to either use pidfd_open
> (which still might be subject to TOCTOU) or keep the old racy interface
> base on pid_t.
>
> The implementation makes sure that kernel must support the complete
> pidfd interface, meaning that waitid (P_PIDFD) should be supported
> (added on Linux 5.4).  It ensures that a non-racy workaround is required
> (such as reading procfs fdinfo pid to use along with wait interfaces).

Sorry, I don't understand the second sentence.

> diff --git a/posix/tst-spawn3.c b/posix/tst-spawn3.c
> index e7ce0fb386..64052dc911 100644
> --- a/posix/tst-spawn3.c
> +++ b/posix/tst-spawn3.c
> @@ -16,6 +16,7 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <assert.h>

Please use TEST_VERIFY_EXIT, see below.

> @@ -75,75 +78,82 @@ do_test (void)
>  	    FAIL_EXIT1 ("create_temp_file: %m");
>  	  break;
>  	}
> -      files[nfiles++] = fd;
> +      files[nfiles] = fd;
>      }
> +  assert (nfiles != 0);

TEST_VERIFY_EXIT (nfiles != 0);

> diff --git a/sysdeps/unix/sysv/linux/bits/spawn_ext.h b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
> index a3aa020d5c..3254cfe9be 100644
> --- a/sysdeps/unix/sysv/linux/bits/spawn_ext.h
> +++ b/sysdeps/unix/sysv/linux/bits/spawn_ext.h
> @@ -37,4 +37,35 @@ extern int posix_spawnattr_setcgroup_np (posix_spawnattr_t *__attr,
>  
>  #endif /* __USE_MISC */
>  
> +#ifdef __USE_GNU

Please use __USE_MISC, so this is available with _DEFAULT_SOURCE (like
the cgroups functions).

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

>    internal_signal_block_all (&args.oldmask);
> @@ -386,13 +399,16 @@ __spawnix (pid_t * pid, const char *file,
>        /* Unsupported flags like CLONE_CLEAR_SIGHAND will be cleared up by
>  	 __clone_internal_fallback.  */
>        .flags = (set_cgroup ? CLONE_INTO_CGROUP : 0)
> +	       | (use_pidfd ? CLONE_PIDFD : 0)
>  	       | CLONE_CLEAR_SIGHAND
>  	       | CLONE_VM
>  	       | CLONE_VFORK,
>        .exit_signal = SIGCHLD,
>        .stack = (uintptr_t) stack,
>        .stack_size = stack_size,
> -      .cgroup = (set_cgroup ? attrp->__cgroup : 0)
> +      .cgroup = (set_cgroup ? attrp->__cgroup : 0),
> +      .pidfd = use_pidfd ? (uintptr_t) &args.pidfd : 0,
> +      .parent_tid = use_pidfd ? (uintptr_t) &args.pidfd : 0,

The .parent_tid line looks wrong?

Thanks,
Florian