<div dir="ltr">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.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, May 6, 2024 at 5:27 PM Adhemerval Zanella <<a href="mailto:adhemerval.zanella@linaro.org">adhemerval.zanella@linaro.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">If the pidfd_spawn / pidfd_spawnp helper process succeeds, but evecve<br>
fails for some reason (either with an invalid/non-existent, memory<br>
allocation, etc.) the resulting pidfd is never closed, nor returned<br>
to caller (so it can call close).<br>
<br>
Since the process creation failed, it should be up to posix_spawn to<br>
also, close the file descriptor in this case (similar to what it<br>
does to reap the process).<br>
<br>
Checked on x86_64-linux-gnu.<br>
---<br>
posix/tst-spawn2.c | 80 +++++++++++++++++++-------------<br>
sysdeps/unix/sysv/linux/spawni.c | 20 +++++---<br>
2 files changed, 61 insertions(+), 39 deletions(-)<br>
<br>
diff --git a/posix/tst-spawn2.c b/posix/tst-spawn2.c<br>
index bb507204a2..b2bad3f1f7 100644<br>
--- a/posix/tst-spawn2.c<br>
+++ b/posix/tst-spawn2.c<br>
@@ -26,6 +26,7 @@<br>
#include <stdio.h><br>
<br>
#include <support/check.h><br>
+#include <support/descriptors.h><br>
#include <tst-spawn.h><br>
<br>
int<br>
@@ -38,38 +39,53 @@ do_test (void)<br>
char * const args[] = { 0 };<br>
PID_T_TYPE pid = -1;<br>
<br>
- int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);<br>
- if (ret != ENOENT)<br>
- {<br>
- errno = ret;<br>
- FAIL_EXIT1 ("posix_spawn: %m");<br>
- }<br>
-<br>
- /* POSIX states the value returned on pid variable in case of an error<br>
- is not specified. GLIBC will update the value iff the child<br>
- execution is successful. */<br>
- if (pid != -1)<br>
- FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);<br>
-<br>
- /* Check if no child is actually created. */<br>
- TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);<br>
- TEST_COMPARE (errno, ECHILD);<br>
-<br>
- /* Same as before, but with posix_spawnp. */<br>
- char *args2[] = { (char*) program, 0 };<br>
-<br>
- ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);<br>
- if (ret != ENOENT)<br>
- {<br>
- errno = ret;<br>
- FAIL_EXIT1 ("posix_spawnp: %m");<br>
- }<br>
-<br>
- if (pid != -1)<br>
- FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);<br>
-<br>
- TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);<br>
- TEST_COMPARE (errno, ECHILD);<br>
+ {<br>
+ struct support_descriptors *descrs = support_descriptors_list ();<br>
+<br>
+ int ret = POSIX_SPAWN (&pid, program, 0, 0, args, environ);<br>
+ if (ret != ENOENT)<br>
+ {<br>
+ errno = ret;<br>
+ FAIL_EXIT1 ("posix_spawn: %m");<br>
+ }<br>
+<br>
+ /* POSIX states the value returned on pid variable in case of an error<br>
+ is not specified. GLIBC will update the value iff the child<br>
+ execution is successful. */<br>
+ if (pid != -1)<br>
+ FAIL_EXIT1 ("posix_spawn returned pid != -1 (%i)", (int) pid);<br>
+<br>
+ /* Check if no child is actually created. */<br>
+ TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);<br>
+ TEST_COMPARE (errno, ECHILD);<br>
+<br>
+ /* Also check if there is no leak descriptors. */<br>
+ support_descriptors_check (descrs);<br>
+ support_descriptors_free (descrs);<br>
+ }<br>
+<br>
+ {<br>
+ /* Same as before, but with posix_spawnp. */<br>
+ char *args2[] = { (char*) program, 0 };<br>
+<br>
+ struct support_descriptors *descrs = support_descriptors_list ();<br>
+<br>
+ int ret = POSIX_SPAWNP (&pid, args2[0], 0, 0, args2, environ);<br>
+ if (ret != ENOENT)<br>
+ {<br>
+ errno = ret;<br>
+ FAIL_EXIT1 ("posix_spawnp: %m");<br>
+ }<br>
+<br>
+ if (pid != -1)<br>
+ FAIL_EXIT1 ("posix_spawnp returned pid != -1 (%i)", (int) pid);<br>
+<br>
+ TEST_COMPARE (WAITID (P_ALL, 0, NULL, WEXITED), -1);<br>
+ TEST_COMPARE (errno, ECHILD);<br>
+<br>
+ support_descriptors_check (descrs);<br>
+ support_descriptors_free (descrs);<br>
+ }<br>
<br>
return 0;<br>
}<br>
diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c<br>
index e8ed2babb9..ca540f11a2 100644<br>
--- a/sysdeps/unix/sysv/linux/spawni.c<br>
+++ b/sysdeps/unix/sysv/linux/spawni.c<br>
@@ -449,13 +449,19 @@ __spawnix (int *pid, const char *file,<br>
caller to actually collect it. */<br>
ec = args.err;<br>
if (ec > 0)<br>
- /* There still an unlikely case where the child is cancelled after<br>
- setting args.err, due to a positive error value. Also there is<br>
- possible pid reuse race (where the kernel allocated the same pid<br>
- to an unrelated process). Unfortunately due synchronization<br>
- issues where the kernel might not have the process collected<br>
- the waitpid below can not use WNOHANG. */<br>
- __waitpid (new_pid, NULL, 0);<br>
+ {<br>
+ /* There still an unlikely case where the child is cancelled after<br>
+ setting args.err, due to a positive error value. Also there is<br>
+ possible pid reuse race (where the kernel allocated the same pid<br>
+ to an unrelated process). Unfortunately due synchronization<br>
+ issues where the kernel might not have the process collected<br>
+ the waitpid below can not use WNOHANG. */<br>
+ __waitpid (new_pid, NULL, 0);<br>
+ /* For pidfd we need to also close the file descriptor for the case<br>
+ where execve fails. */<br>
+ if (use_pidfd)<br>
+ __close (args.pidfd);<br>
+ }<br>
}<br>
else<br>
ec = errno;<br>
-- <br>
2.43.0<br>
<br>
</blockquote></div>