Hi Omar,
Apologies for the late reply.
Omar Polo <o...@omarpolo.com> 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:
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)
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’.
