[Bug libc/30349] New: Support returning a pidfd from posix_spawn()

[Bug libc/30349] New: Support returning a pidfd from posix_spawn()

[bugzilla at tecnocode dot co.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

            Bug ID: 30349
           Summary: Support returning a pidfd from posix_spawn()
           Product: glibc
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: bugzilla at tecnocode dot co.uk
                CC: drepper.fsp at gmail dot com
  Target Milestone: ---

pidfd allows a process to keep a race-free handle to a child process, avoiding
the problem of PID recycling meaning that a pid_t no longer refers to the
process you thought it did.

We’d like to be able to use pidfds in GLib for race-free signalling of
subprocesses. GLib currently uses posix_spawn() to spawn those subprocesses,
though, and it cannot return a pidfd.

Could glibc please add a variant of posix_spawn() which returns a pidfd to the
caller? I don’t know of the best API for this, but it could either be a new
posix_spawn_with_pidfd() function, or be the existing posix_spawn() but somehow
return a pidfd via a posix_spawnattr_t.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[bugzilla at tecnocode dot co.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

Philip Withnall <bugzilla at tecnocode dot co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[bugzilla at tecnocode dot co.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #1 from Philip Withnall <bugzilla at tecnocode dot co.uk> ---
Relevant discussion in GLib’s issue tracker:
https://gitlab.gnome.org/GNOME/glib/-/issues/1866

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |adhemerval.zanella at linaro dot o
                   |                            |rg

--- Comment #2 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
I think it should be a reasonable interface for Linux.  I think the most
straightforward interface would be:

 int posix_spawn_pidfd_ex (int *restrict pidfd, const char *restrict path,
                           const posix_spawn_file_actions_t *restrict
file_actions,
                           const posix_spawnattr_t *restrict attrp,
                           char *const argv[restrict_arr],
                           char *const envp[restrict_arr])

Since returning the value on posix_spawnattr_t would require removing the const
from it, which is not orthogonal to posix_spawn/posix_spawnp.

It will only work on Linux 5.2; but since it is an extension I think it should
reasonable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #3 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Another question is if would make sense to also provide the posix_spawnp
variant as well, to be orthogonal to POSIX interface.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #4 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Another issue is if the new interface would be required to also return the
pid_t from he spawned process.  Returning it would allow it to work with the
default wait interfaces (waitpid, etc.); and it would avoid the possible issue
of using this interface with a kernel between 5.2 and 5.4 (since waitid only
support for 5.4+).

My take is to not support return the pid_t (since posix_spawn{p} already
provides it), but now that we don't effoce kernel versions I don't have a good
solution for the case kernel does not support waitid (P_PIDFD).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #5 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Another corner case, stressed by tst-spawn3.c, is when the process has already
reached its maximum number of file descriptors (RLIMIT_NOFILE). Without pidfd,
posix_spawn would create the process, while now clone (CLONE_PIDFD) fails.

It is expected, but it might be a caveat while using the interface.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #6 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(In reply to Adhemerval Zanella from comment #2)
> I think it should be a reasonable interface for Linux.  I think the most
> straightforward interface would be:
>  
>  int posix_spawn_pidfd_ex (int *restrict pidfd, const char *restrict path,
>                            const posix_spawn_file_actions_t *restrict
> file_actions,
>                            const posix_spawnattr_t *restrict attrp,
>                            char *const argv[restrict_arr],
>                            char *const envp[restrict_arr])

Another option, which would not require adding a new symbol, would to add a new
posix_spawnattr_t attribute (POSIX_SPAWN_PIDFD), which would make posix_spawn
return the pidfd on the 'pid' argument.  It should be safe at least on Linux,
where pid_t and int are interchangeable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #7 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
After discussion with other maintainers, it seems that it would be better not
to reuse the posix_spawn and posix_spawnp interface (with a new flag) and
instead add different interfaces.  Florian has raised the possible issue with
language bindings that might track the argument lifetime and mixing pid_t and
file descriptors might generate some problems in such scenarios.

And since we already provide wrappers over the Linux pidfd syscall, Florian
also suggested using the same name convention.  The proposed interfaces are:

extern int pidfd_spawn (int *__restrict __pid,
                        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])
    __nonnull ((2, 5));

extern int pidfd_spawnp (int *__restrict __pid,
                         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])
    __nonnull ((2, 5));

I decided to reuse the posix_spawn attribute and file actions to avoid either
to rehash posix_spawn API or add a new one.  This new API will be provided by
spawn.h.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #8 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Proposed patch sent upstream -
https://patchwork.sourceware.org/project/glibc/list/?series=19063

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[bugzilla at tecnocode dot co.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #9 from Philip Withnall <bugzilla at tecnocode dot co.uk> ---
I’ve looked over the latest patches in
https://patchwork.sourceware.org/project/glibc/list/?series=19143, and it all
looks good to me! I focused on looking at whether it would cover GLib’s needs,
and whether the new API made sense and felt natural and complete. I have not
done a line-by-line review or looked in detail at the /proc parsing code.

So, from the point of view of GLib, the proposed APIs look great. Thanks a lot
for working on them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[bluca at debian dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

Luca Boccassi <bluca at debian dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bluca at debian dot org

--- Comment #10 from Luca Boccassi <bluca at debian dot org> ---
The new API looks great also from systemd's point of view, thanks for your
work, looking forward to using these!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

Adhemerval Zanella <adhemerval.zanella at linaro dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |2.39
           Assignee|unassigned at sourceware dot org   |adhemerval.zanella at linaro dot o
                   |                            |rg

--- Comment #11 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
Implemented on 2.39 (0d6f9f626521678f330f8bfee89e1cdb7e2b1062) by
pidfd_spawn/pidfd_spawnp.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/30349] Support returning a pidfd from posix_spawn()

[bugzilla at tecnocode dot co.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=30349

--- Comment #12 from Philip Withnall <bugzilla at tecnocode dot co.uk> ---
Fantastic! Thanks for your work on this.

-- 
You are receiving this mail because:
You are on the CC list for the bug.