| 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 |
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 > >
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 --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;