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

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).

This patch also changes the waitpid with waitid (P_PIDFD) for pidfd
case, to avoid a possible pid re-use.

Checked on x86_64-linux-gnu.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
This commit is contained in:
Adhemerval Zanella 2024-05-06 13:20:56 -03:00
parent 17a293c5fa
commit c90cfce849
2 changed files with 58 additions and 33 deletions

View File

@ -26,6 +26,7 @@
#include <stdio.h> #include <stdio.h>
#include <support/check.h> #include <support/check.h>
#include <support/descriptors.h>
#include <tst-spawn.h> #include <tst-spawn.h>
int int
@ -38,6 +39,9 @@ do_test (void)
char * const args[] = { 0 }; char * const args[] = { 0 };
PID_T_TYPE pid = -1; PID_T_TYPE pid = -1;
{
struct support_descriptors *descrs = support_descriptors_list ();
int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ); int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);
if (ret != ENOENT) if (ret != ENOENT)
{ {
@ -55,10 +59,18 @@ do_test (void)
TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1); TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
TEST_COMPARE (errno, ECHILD); 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. */ /* Same as before, but with posix_spawnp. */
char *args2[] = { (char*) program, 0 }; char *args2[] = { (char*) program, 0 };
ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ); struct support_descriptors *descrs = support_descriptors_list ();
int ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);
if (ret != ENOENT) if (ret != ENOENT)
{ {
errno = ret; errno = ret;
@ -71,6 +83,10 @@ do_test (void)
TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1); TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);
TEST_COMPARE (errno, ECHILD); TEST_COMPARE (errno, ECHILD);
support_descriptors_check (descrs);
support_descriptors_free (descrs);
}
return 0; return 0;
} }

View File

@ -449,13 +449,22 @@ __spawnix (int *pid, const char *file,
caller to actually collect it. */ caller to actually collect it. */
ec = args.err; ec = args.err;
if (ec > 0) if (ec > 0)
{
/* There still an unlikely case where the child is cancelled after /* There still an unlikely case where the child is cancelled after
setting args.err, due to a positive error value. Also there is setting args.err, due to a positive error value. Also there is
possible pid reuse race (where the kernel allocated the same pid possible pid reuse race (where the kernel allocated the same pid
to an unrelated process). Unfortunately due synchronization to an unrelated process). Unfortunately due synchronization
issues where the kernel might not have the process collected issues where the kernel might not have the process collected
the waitpid below can not use WNOHANG. */ the waitpid below can not use WNOHANG. */
__waitpid (new_pid, NULL, 0); __waitid (use_pidfd ? P_PIDFD : P_PID,
use_pidfd ? args.pidfd : new_pid,
NULL,
WEXITED);
/* 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 else
ec = errno; ec = errno;