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

[Peter Cawley]

RE the spawni.c change, should it use __close_nocancel rather than __close?
Cancellation should be disabled at this point in time due to earlier logic,
but __spawni_child still seems to err on the side of caution and
use __close_nocancel for the closing that it does, and it seems prudent to
be consistent.

On Mon, May 6, 2024 at 5:27 PM Adhemerval Zanella <
adhemerval.zanella@linaro.org> 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.
> ---
>  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..ca540f11a2 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 (args.pidfd);
> +       }
>      }
>    else
>      ec = errno;
> --
> 2.43.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20240506/95cc87dd/attachment-0001.htm>