diff mbox series

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

Message ID 20240506162658.1571389-1-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series posix: Fix pidfd_spawn/pidfd_spawnp leak if execve fails (BZ 31695) | expand

Commit Message

Adhemerval Zanella Netto May 6, 2024, 4:26 p.m. UTC
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(-)

Comments

Peter Cawley May 6, 2024, 4:34 p.m. UTC | #1
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
>
>
Adhemerval Zanella Netto May 6, 2024, 5:24 p.m. UTC | #2
It should not matter, the _nocancel calls on spawni in fact came from
the previous implementation (sysdeps/posix/spawni.c) that did not disable
the cancellation. It used to work due the limited number of flags and
file operations supported.

But I agree that it does not hurt also, and I think we should use
__close_nocancel_nostatus to make it explicit that the function does
not really care for an eventual close failure (meaning that either
something is really funny with kernel or some thread has closed the
file descriptor already).


On 06/05/24 13:34, Peter Cawley wrote:
> 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 <mailto: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
>
diff mbox series

Patch

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;