| Message ID | 20240506172816.1661462-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] posix: Fix pidfd_spawn/pidfd_spawnp leak if execve fails (BZ 31695) | expand |
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;
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> 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; >
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; >
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;