bug#61095: possible misuse of posix_spawn API on non-linux OSes

["Ludovic Courtès" <ludo@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

Hi Omar,

Apologies for the late reply.

Omar Polo <op@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 3545 bytes --]

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)

[-- Attachment #3: Type: text/plain, Size: 1152 bytes --]


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’.