bug#61095: possible misuse of posix_spawn API on non-linux OSes
Ludovic Courtès <[email protected]>
| Newsgroups | gmane.lisp.guile.bugs |
|---|---|
| Message-ID | <[email protected]> |
Hi Omar, Apologies for the late reply. Omar Polo <[email protected]> skribis: > I've noticed that test-system-cmds fails on OpenBSD-CURRENT while > testing the update to guile 3.0.9: > > test-system-cmds: system* exit status was 127 rather than 42 > FAIL: test-system-cmds We’re seeing the same failure on GNU/Hurd: https://issues.guix.gnu.org/61079 > Actually I can avoid the EBADF by checking that the fd is 'live' with > something like fstat: > > [[[ > > Index: libguile/posix.c > --- libguile/posix.c.orig > +++ libguile/posix.c > @@ -1325,8 +1325,12 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0, > static void > close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd) > { > - while (--max_fd > 2) > - posix_spawn_file_actions_addclose (actions, max_fd); > + struct stat sb; > + max_fd = getdtablecount(); > + while (--max_fd > 2) { > + if (fstat(max_fd, &sb) != -1) > + posix_spawn_file_actions_addclose (actions, max_fd); > + } > } I came up with the following patch: Could you confirm that it works on OpenBSD and that there’s no performance regression? Andrew: it removes the /proc/self/fd loop you added to fix <https://bugs.gnu.org/59321>, but it reduces the number of ‘close’ calls in the child. Could you check whether that’s okay performance-wise? Eventually I plan to use ‘posix_spawn_file_actions_addclosefrom_np’ on glibc >= 2.34, but I have yet to test it. That will be the best solution. Josselin: I simplified the ‘dup2’ logic somewhat. Feedback welcome! > The regress passes and while this workaround may be temporarly > acceptable I -personally- don't like it much. There's a reason guile > can't set CLOEXEC for all the file descriptors > 2 obtained via open, > socket, pipe, ... like perl -for example- does? Guile does that for file descriptors it opens internally, but applications using ‘open-file’ without the recently-added “e” flag, or ‘socket’ without ‘SOCK_CLOEXEC’, etc., end up with more file descriptors that need to be taken care of. I wish the default were close-on-exec, but we’re not there yet. Thanks, Ludo’.
(unnamed)
(text/x-patch, 3.5 KB)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3a8be94e4..cde199888 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1322,39 +1322,18 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
-static void
-close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
-{
- while (--max_fd > 2)
- posix_spawn_file_actions_addclose (actions, max_fd);
-}
-
static void
close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
{
- DIR *dirp;
- struct dirent *d;
- int fd;
-
- /* Try to use the platform-specific list of open file descriptors, so
- we don't need to use the brute force approach. */
- dirp = opendir ("/proc/self/fd");
-
- if (dirp == NULL)
- return close_inherited_fds_slow (actions, max_fd);
-
- while ((d = readdir (dirp)) != NULL)
+ while (--max_fd > 2)
{
- fd = atoi (d->d_name);
-
- /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
- if (fd <= 2)
- continue;
-
- posix_spawn_file_actions_addclose (actions, fd);
+ /* Adding invalid file descriptors to an 'addclose' action leads
+ to 'posix_spawn' failures on some operating systems:
+ <https://bugs.gnu.org/61095>. Hence the extra check. */
+ int flags = fcntl (max_fd, F_GETFD, NULL);
+ if ((flags >= 0) && ((flags & FD_CLOEXEC) == 0))
+ posix_spawn_file_actions_addclose (actions, max_fd);
}
-
- closedir (dirp);
}
static pid_t
@@ -1366,6 +1345,26 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
posix_spawn_file_actions_t actions;
posix_spawnattr_t *attrp = NULL;
+ posix_spawn_file_actions_init (&actions);
+
+ /* Duplicate IN, OUT, and ERR unconditionally to clear their
+ FD_CLOEXEC flag, if any. */
+ posix_spawn_file_actions_adddup2 (&actions, in, STDIN_FILENO);
+ posix_spawn_file_actions_adddup2 (&actions, out, STDOUT_FILENO);
+ posix_spawn_file_actions_adddup2 (&actions, err, STDERR_FILENO);
+
+ /* TODO: Use 'closefrom' where available. */
+#if 0
+ /* Version 2.34 of the GNU libc provides this function. */
+ posix_spawn_file_actions_addclosefrom_np (&actions, 3);
+#else
+ if (in > 2)
+ posix_spawn_file_actions_addclose (&actions, in);
+ if (out > 2 && out != in)
+ posix_spawn_file_actions_addclose (&actions, out);
+ if (err > 2 && err != out && err != in)
+ posix_spawn_file_actions_addclose (&actions, err);
+
int max_fd = 1024;
#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
@@ -1376,31 +1375,8 @@ do_spawn (char *exec_file, char **exec_argv, char **exec_env,
}
#endif
- posix_spawn_file_actions_init (&actions);
-
- int free_fd_slots = 0;
- int fd_slot[3];
-
- for (int fdnum = 3; free_fd_slots < 3 && fdnum < max_fd; fdnum++)
- {
- if (fdnum != in && fdnum != out && fdnum != err)
- {
- fd_slot[free_fd_slots] = fdnum;
- free_fd_slots++;
- }
- }
-
- /* Move the fds out of the way, so that duplicate fds or fds equal
- to 0, 1, 2 don't trample each other */
-
- posix_spawn_file_actions_adddup2 (&actions, in, fd_slot[0]);
- posix_spawn_file_actions_adddup2 (&actions, out, fd_slot[1]);
- posix_spawn_file_actions_adddup2 (&actions, err, fd_slot[2]);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[0], 0);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
- posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
-
close_inherited_fds (&actions, max_fd);
+#endif
int res = -1;
if (spawnp)