Re: [PATCH v4 1/3] posix: Add pidfd_spawn and pidfd_spawnp (BZ# 30349)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 22/05/23 07:41, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> Returning a pidfd allows a process to keep a race-free handle to a child
>> process, otherwise the caller will need to either use pidfd_open (which
>> still might be subject to TOCTOU) or keep using the old racy interface.
>>
>> 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 ensure that non racy workaround is required
>> (such as reading procfs fdinfo pid to use along with old wait interfaces).
>>  If kernel does not have the required support the interface returns ENOSYS.
> 
> “It avoids workarounds with race conditions”?

Ack.

> 
>> These interfaces are similar to the posix_spawn and posix_spawnp, with
>> the only different diferent being it returns a process file descriptor
>> (int) instead of process ID (pid_t).  Their prototypes are:
>>
>>   int pidfd_spawn (int *restrict pidfd,
>>  		   const char *restrict file,
>>   		   const posix_spawn_file_actions_t *restrict facts,
>>   		   const posix_spawnattr_t *restrict attrp,
>>   		   char *const argv[restrict],
>>   		   char *const envp[restrict])
>>
>>   int pidfd_spawnp (int *restrict pidfd,
>>  		    const char *restrict path,
>>   		    const posix_spawn_file_actions_t *restrict facts,
>>   		    const posix_spawnattr_t *restrict attrp,
>>   		    char *const argv[restrict_arr],
>>   		    char *const envp[restrict_arr]);
> 
> Should we return the pid_t value as we..

That's a key point where I was not sure and ended up to omit since the
patchset also adds pidfd_getpid (so if call is successful pidfd_getpid
will get you the pid).

Returning the pid will also deviate the interface for posix_spawnp, which
means that either we will need to add the pid_t as an additional input/output
argument or change the interface to return errors through errno.

> 
>> A new symbol is used instead of a posix_spawn extension to avoid possible
>> issue with language bindings that might track the return argument
>> lifetime.  Although, on Linux pid_t and int are interchangeable, POSIX
>> only state that pid_t should be a signed interger.
>>
>> Both symbols reuse the posix_spawn posix_spawn_file_actions_t and
>> posix_spawnattr_t, to void rehash posix_spawn API or add a new one.
>>  It also mean that both interfaces support the same attribute and
>> file actions, and a new flag or file actions on posix_spawn is also
>> added automatically for pidfd_spawn.
> 
> Same question as with pidfd_fork: how does this interact with SIGCHLD,
> and can the PID be used with waitpid?

It follows posix_spawn semantic to setup SIGCHLD on lower bits of clone
flags.  I think it should be feasible to add an extra posix_spawn flag
to remove it.

> 
>> diff --git a/posix/spawn.h b/posix/spawn.h
>> index 0a6a070523..2e20991627 100644
>> --- a/posix/spawn.h
>> +++ b/posix/spawn.h
>> @@ -229,6 +229,8 @@ posix_spawn_file_actions_addtcsetpgrp_np (posix_spawn_file_actions_t *,
>>  
>>  #endif /* __USE_MISC */
>>  
>> +#include <bits/spawn_ext.h>
>> +
>>  __END_DECLS
> 
> Nested __BEGIN_DECLS.  You should probably move the #include after
> __END_DECLS.

Ack.

> 
>> diff --git a/sysdeps/unix/sysv/linux/clone-pidfd-support.c b/sysdeps/unix/sysv/linux/clone-pidfd-support.c
>> new file mode 100644
>> index 0000000000..4bf2317c17
>> --- /dev/null
>> +++ b/sysdeps/unix/sysv/linux/clone-pidfd-support.c
> 
>> +         Also tThe waitid is a cancellation entrypoint, so issue the syscall
>> +	 directly.  */
> 
> Typo: “tThe”
> 
> Thanks,
> Florian
>