[PATCH v2] posix: Fix pidfd_spawn/pidfd_spawnp leak if execve fails (BZ 31695)

[Adhemerval Zanella Netto]

Indeed using waitid with P_PIDFD is the correct approach, I will update
the patch.

On 20/05/24 13:54, Peter Cawley wrote:
> For whatever it is worth, the patch looks good to me for addressing the reported bug.
> 
> That said, as we're in the area, we could potentially go further and replace the __waitpid with a P_PIDFD waitid when we're in the use_pidfd case, thereby closing the pid reuse race described in the comment.
> 
> On Mon, May 20, 2024 at 5:47 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
> 
>     Ping.
> 
>     On 06/05/24 14:27, Adhemerval Zanella wrote:
>     > If the pidfd_spawn / pidfd_spawnp helper process succeeds, but evecve
>     > fails for some reason (either with an invalid/non-existent, memory
>     > allocation, etc.) the resulting pidfd is never closed, nor returned
>     > to caller (so it can call close).
>     >
>     > Since the process creation failed, it should be up to posix_spawn to
>     > also, close the file descriptor in this case (similar to what it
>     > does to reap the process).
>     >
>     > Checked on x86_64-linux-gnu.
>     > --
>     > Changes from v1:
>     > - Use __close_nocancel_nostatus instead of __close.
>     > ---
>     >  posix/tst-spawn2.c               | 80 +++++++++++++++++++-------------
>     >  sysdeps/unix/sysv/linux/spawni.c | 20 +++++---
>     >  2 files changed, 61 insertions(+), 39 deletions(-)
>     >
>     > diff --git a/posix/tst-spawn2.c b/posix/tst-spawn2.c
>     > index bb507204a2..b2bad3f1f7 100644
>     > --- a/posix/tst-spawn2.c
>     > +++ b/posix/tst-spawn2.c
>     > @@ -26,6 +26,7 @@
>     >  #include <stdio.h>
>     > 
>     >  #include <support/check.h>
>     > +#include <support/descriptors.h>
>     >  #include <tst-spawn.h>
>     > 
>     >  int
>     > @@ -38,38 +39,53 @@ do_test (void)
>     >    char * const args[] = { 0 };
>     >    PID_T_TYPE pid = -1;
>     > 
>     > -  int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);
>     > -  if (ret != ENOENT)
>     > -    {
>     > -      errno = ret;
>     > -      FAIL_EXIT1 ("posix_spawn: %m");
>     > -    }
>     > -
>     > -  /* POSIX states the value returned on pid variable in case of an error
>     > -     is not specified.  GLIBC will update the value iff the child
>     > -     execution is successful.  */
>     > -  if (pid != -1)
>     > -    FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
>     > -
>     > -  /* Check if no child is actually created.  */
>     > -  TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
>     > -  TEST_COMPARE (errno, ECHILD);
>     > -
>     > -  /* Same as before, but with posix_spawnp.  */
>     > -  char *args2[] = { (char*) program, 0 };
>     > -
>     > -  ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);
>     > -  if (ret != ENOENT)
>     > -    {
>     > -      errno = ret;
>     > -      FAIL_EXIT1 ("posix_spawnp: %m");
>     > -    }
>     > -
>     > -  if (pid != -1)
>     > -    FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
>     > -
>     > -  TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
>     > -  TEST_COMPARE (errno, ECHILD);
>     > +  {
>     > +    struct support_descriptors *descrs = support_descriptors_list ();
>     > +
>     > +    int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);
>     > +    if (ret != ENOENT)
>     > +      {
>     > +     errno = ret;
>     > +     FAIL_EXIT1 ("posix_spawn: %m");
>     > +      }
>     > +
>     > +    /* POSIX states the value returned on pid variable in case of an error
>     > +       is not specified.  GLIBC will update the value iff the child
>     > +       execution is successful.  */
>     > +    if (pid != -1)
>     > +      FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);
>     > +
>     > +    /* Check if no child is actually created.  */
>     > +    TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
>     > +    TEST_COMPARE (errno, ECHILD);
>     > +
>     > +    /* Also check if there is no leak descriptors.  */
>     > +    support_descriptors_check (descrs);
>     > +    support_descriptors_free (descrs);
>     > +  }
>     > :+
>     > +  {
>     > +    /* Same as before, but with posix_spawnp.  */
>     > +    char *args2[] = { (char*) program, 0 };
>     > +
>     > +    struct support_descriptors *descrs = support_descriptors_list ();
>     > +
>     > +    int ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);
>     > +    if (ret != ENOENT)
>     > +      {
>     > +     errno = ret;
>     > +     FAIL_EXIT1 ("posix_spawnp: %m");
>     > +      }
>     > +
>     > +    if (pid != -1)
>     > +      FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);
>     > +
>     > +    TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
>     > +    TEST_COMPARE (errno, ECHILD);
>     > +
>     > +    support_descriptors_check (descrs);
>     > +    support_descriptors_free (descrs);
>     > +  }
>     > 
>     >    return 0;
>     >  }
>     > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c
>     > index e8ed2babb9..1556dd17e0 100644
>     > --- a/sysdeps/unix/sysv/linux/spawni.c
>     > +++ b/sysdeps/unix/sysv/linux/spawni.c
>     > @@ -449,13 +449,19 @@ __spawnix (int *pid, const char *file,
>     >        caller to actually collect it.  */
>     >        ec = args.err;
>     >        if (ec > 0)
>     > -     /* There still an unlikely case where the child is cancelled after
>     > -        setting args.err, due to a positive error value.  Also there is
>     > -        possible pid reuse race (where the kernel allocated the same pid
>     > -        to an unrelated process).  Unfortunately due synchronization
>     > -        issues where the kernel might not have the process collected
>     > -        the waitpid below can not use WNOHANG.  */
>     > -     __waitpid (new_pid, NULL, 0);
>     > +     {
>     > +       /* There still an unlikely case where the child is cancelled after
>     > +          setting args.err, due to a positive error value.  Also there is
>     > +          possible pid reuse race (where the kernel allocated the same pid
>     > +          to an unrelated process).  Unfortunately due synchronization
>     > +          issues where the kernel might not have the process collected
>     > +          the waitpid below can not use WNOHANG.  */
>     > +       __waitpid (new_pid, NULL, 0);
>     > +       /* For pidfd we need to also close the file descriptor for the case
>     > +          where execve fails.  */
>     > +       if (use_pidfd)
>     > +         __close_nocancel_nostatus (args.pidfd);
>     > +     }
>     >      }
>     >    else
>     >      ec = errno;
>