bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Ludovic Courtès]

Hi Josselin,

Josselin Poiret  skribis:

> Here is hopefully the last reroll of this patchset.  First of all, I did not
> include the gnulib patch again because it still applies cleanly and it is
> extremely large, but it should be applied before those 3 patches.

Yay!

> The first two patches should be applied on the current major release, while 
> the
> third one should be applied on the next major release to finish the migration 
> to
> spawn.

I pushed it to ‘wip-posix-spawn’ along with fixups I’m proposing, mostly
along the lines of what I suggested in
:

  • Avoiding the extra ‘fork’ in ‘system*’ upon error;

  • Keep ‘scm_spawn_process’ internal.

I also added Andrew Whatson’s patch from
.

If that’s fine with you, I can squash the “fixup!” commits and merge the
branch.  Let me know!

Earlier we agreed it’d be nice to expose ‘spawn*’/‘primitive-spawn’.  I
still think it’s a good idea, but the interface would need some work IMO
to be more generally useful.  In essence, we could provide something
similar to ‘fork+exec-command’ in the Shepherd, where one can pass
environment variables, stdin/stdout/stderr, etc., all that with keyword
arguments and reasonable defaults.

It’s a bit of extra work though so we can discuss that later,
separately.

Thank you!

Ludo’.

bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly

[Bug reports for GUILE, GNU's Ubiquitous Extension Language]

Hi Ludo, thanks for the quick review and fixes.

> I pushed it to ‘wip-posix-spawn’ along with fixups I’m proposing, mostly
> along the lines of what I suggested in
> :

Nice but also see below.

> I also added Andrew Whatson’s patch from
> .

Great, hadn't see that one go by!

> If that’s fine with you, I can squash the “fixup!” commits and merge the
> branch.  Let me know!
>
> Earlier we agreed it’d be nice to expose ‘spawn*’/‘primitive-spawn’.  I
> still think it’s a good idea, but the interface would need some work IMO
> to be more generally useful.  In essence, we could provide something
> similar to ‘fork+exec-command’ in the Shepherd, where one can pass
> environment variables, stdin/stdout/stderr, etc., all that with keyword
> arguments and reasonable defaults.

I've just polished it up a bit: the `spawn*` procedure defined in C now
takes another argument, a list of environment variables.  I think this
interface is good enough to cover most use cases, if anyone needs
something more complicated they should go through their own C code.
I've added a convenience module (ice-9 spawn) with a `spawn` procedure
in it, which takes an optional argument list which defaults to just the
executable, and optional environment variables as well as in, out and
err ports.  I also think everything in (ice-9 popen) should be migrated
on the next major release, as well as being re-implemented in terms of
`spawn` purely, so that the change is immediately noticeable.

We're reaching the bike-shedding time now, but IMHO having such a
`spawn*` exposed to the user seems fine, it's a pretty simple "raw"
interface with fdes, and there is a convenience `spawn` function that is
nicer for users.

Do we need to add a documentation page as well?

Best,
-- 
Josselin Poiret

bug#52835: [PATCH v7 2/2] Make system* and piped-process internally use spawn.

[Bug reports for GUILE, GNU's Ubiquitous Extension Language]

* libguile/posix.c (scm_system_star, scm_piped_process): Use do_spawn.
(start_child): Remove function.

Co-authored-by: Ludovic Courtès 
---
 libguile/posix.c | 233 ---
 1 file changed, 78 insertions(+), 155 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 52dc11e57..ecc2b186e 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -77,6 +77,7 @@
 #include "threads.h"
 #include "values.h"
 #include "vectors.h"
+#include "verify.h"
 #include "version.h"
 
 #if (SCM_ENABLE_DEPRECATED == 1)
@@ -95,6 +96,13 @@
 # define WIFEXITED(stat_val) (((stat_val) & 255) == 0)
 #endif
 
+#ifndef W_EXITCODE
+/* Macro for constructing a status value.  Found in glibc.  */
+# define W_EXITCODE(ret, sig)   ((ret) << 8 | (sig))
+#endif
+verify (WEXITSTATUS (W_EXITCODE (127, 0)) == 127);
+
+
 #include 
 
 #ifdef HAVE_GRP_H
@@ -1308,125 +1316,6 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
 #undef FUNC_NAME
 #endif /* HAVE_FORK */
 
-#ifdef HAVE_FORK
-/* 'renumber_file_descriptor' is a helper function for 'start_child'
-   below, and is specialized for that particular environment where it
-   doesn't make sense to report errors via exceptions.  It uses dup(2)
-   to duplicate the file descriptor FD, closes the original FD, and
-   returns the new descriptor.  If dup(2) fails, print an error message
-   to ERR and abort.  */
-static int
-renumber_file_descriptor (int fd, int err)
-{
-  int new_fd;
-
-  do
-new_fd = dup (fd);
-  while (new_fd == -1 && errno == EINTR);
-
-  if (new_fd == -1)
-{
-  /* At this point we are in the child process before exec.  We
- cannot safely raise an exception in this environment.  */
-  const char *msg = strerror (errno);
-  fprintf (fdopen (err, "a"), "start_child: dup failed: %s\n", msg);
-  _exit (127);  /* Use exit status 127, as with other exec errors. */
-}
-
-  close (fd);
-  return new_fd;
-}
-#endif /* HAVE_FORK */
-
-#ifdef HAVE_FORK
-#define HAVE_START_CHILD 1
-/* Since Guile uses threads, we have to be very careful to avoid calling
-   functions that are not async-signal-safe in the child.  That's why
-   this function is implemented in C.  */
-static pid_t
-start_child (const char *exec_file, char **exec_argv,
-int reading, int c2p[2], int writing, int p2c[2],
- int in, int out, int err)
-{
-  int pid;
-  int max_fd = 1024;
-
-#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
-  {
-struct rlimit lim = { 0, 0 };
-if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
-  max_fd = lim.rlim_cur;
-  }
-#endif
-
-  pid = fork ();
-
-  if (pid != 0)
-/* The parent, with either and error (pid == -1), or the PID of the
-   child.  Return directly in either case.  */
-return pid;
-
-  /* The child.  */
-  if (reading)
-close (c2p[0]);
-  if (writing)
-close (p2c[1]);
-
-  /* Close all file descriptors in ports inherited from the parent
- except for in, out, and err.  Heavy-handed, but robust.  */
-  while (max_fd--)
-if (max_fd != in && max_fd != out && max_fd != err)
-  close (max_fd);
-
-  /* Ignore errors on these open() calls.  */
-  if (in == -1)
-in = open ("/dev/null", O_RDONLY);
-  if (out == -1)
-out = open ("/dev/null", O_WRONLY);
-  if (err == -1)
-err = open ("/dev/null", O_WRONLY);
-
-  if (in > 0)
-{
-  if (out == 0)
-out = renumber_file_descriptor (out, err);
-  if (err == 0)
-err = renumber_file_descriptor (err, err);
-  do dup2 (in, 0); while (errno == EINTR);
-  close (in);
-}
-  if (out > 1)
-{
-  if (err == 1)
-err = renumber_file_descriptor (err, err);
-  do dup2 (out, 1); while (errno == EINTR);
-  if (out > 2)
-close (out);
-}
-  if (err > 2)
-{
-  do dup2 (err, 2); while (errno == EINTR);
-  close (err);
-}
-
-  execvp (exec_file, exec_argv);
-
-  /* The exec failed!  There is nothing sensible to do.  */
-  {
-const char *msg = strerror (errno);
-fprintf (fdopen (2, "a"), "In execvp of %s: %s\n",
- exec_file, msg);
-  }
-
-  /* Use exit status 127, like shells in this case, as per POSIX
- 
.
  */
-  _exit (127);
-
-  /* Not reached.  */
-  return -1;
-}
-#endif
-
 static pid_t
 do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, 
int err)
 {
@@ -1508,18 +1397,18 @@ SCM_DEFINE (scm_spawn_process, "spawn*", 6, 0, 0,
 }
 #undef FUNC_NAME
 
-#ifdef HAVE_START_CHILD
-static SCM
-scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
+#ifdef HAVE_FORK
+static int
+piped_process (pid_t *pid, SCM prog, SCM args, SCM from, SCM to)
 #define FUNC_NAME "piped-process"
 {
   int reading, writing;
   int c2p[2]; /* Child to parent.  */
   int p2c[2]; /* Parent to child.  */
   int in = -1, out = -1, err = -1;
-  int pid;
   char *exec_file;
   char **exec_argv;
+  char **exec_env 

bug#52835: [PATCH v7 1/2] Add spawn* and spawn.

[Bug reports for GUILE, GNU's Ubiquitous Extension Language]

* libguile/posix.c: Include spawn.h from Gnulib.
(do_spawn, scm_spawn_process): New functions.
* module/ice-9/spawn.scm: New file
(spawn): New procedure.
---
 libguile/posix.c   | 82 ++
 libguile/posix.h   |  2 ++
 module/ice-9/spawn.scm | 54 
 3 files changed, 138 insertions(+)
 create mode 100644 module/ice-9/spawn.scm

diff --git a/libguile/posix.c b/libguile/posix.c
index b5352c2c4..52dc11e57 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef HAVE_SCHED_H
 # include 
@@ -1426,6 +1427,87 @@ start_child (const char *exec_file, char **exec_argv,
 }
 #endif
 
+static pid_t
+do_spawn (char *exec_file, char **exec_argv, char **exec_env, int in, int out, 
int err)
+{
+  pid_t pid = -1;
+
+  posix_spawn_file_actions_t actions;
+  posix_spawnattr_t *attrp = NULL;
+
+  int max_fd = 1024;
+
+#if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
+  {
+struct rlimit lim = { 0, 0 };
+if (getrlimit (RLIMIT_NOFILE, &lim) == 0)
+  max_fd = lim.rlim_cur;
+  }
+#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);
+
+  while (--max_fd > 2)
+posix_spawn_file_actions_addclose (&actions, max_fd);
+
+  if (posix_spawnp (&pid, exec_file, &actions, attrp, exec_argv, exec_env) != 
0)
+return -1;
+
+  return pid;
+}
+
+SCM_DEFINE (scm_spawn_process, "spawn*", 6, 0, 0,
+(SCM prog, SCM args, SCM env, SCM in, SCM out, SCM err),
+"Spawns a new child process executing @var{prog} with arguments\n"
+"@var{args}, with its standard input, output and error file 
descriptors\n"
+"set to @var{in}, @var{out}, @var{err}, and environment to 
@var{env}.")
+#define FUNC_NAME s_scm_spawn_process
+{
+  int pid;
+  char *exec_file;
+  char **exec_argv;
+  char **exec_env;
+
+  exec_file = scm_to_locale_string (prog);
+  exec_argv = scm_i_allocate_string_pointers (args);
+  exec_env = scm_i_allocate_string_pointers (env);
+
+  pid = do_spawn (exec_file, exec_argv, exec_env,
+  scm_to_int (in),
+  scm_to_int (out),
+  scm_to_int (err));
+
+  free (exec_file);
+
+  if (pid == -1)
+SCM_SYSERROR;
+
+  return scm_from_int (pid);
+}
+#undef FUNC_NAME
+
 #ifdef HAVE_START_CHILD
 static SCM
 scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
diff --git a/libguile/posix.h b/libguile/posix.h
index 6504eaea8..35c502bc1 100644
--- a/libguile/posix.h
+++ b/libguile/posix.h
@@ -69,6 +69,8 @@ SCM_API SCM scm_tmpnam (void);
 SCM_API SCM scm_tmpfile (void);
 SCM_API SCM scm_open_pipe (SCM pipestr, SCM modes);
 SCM_API SCM scm_close_pipe (SCM port);
+SCM_API SCM scm_spawn_process (SCM prog, SCM args, SCM env,
+   SCM in, SCM out, SCM err);
 SCM_API SCM scm_system_star (SCM cmds);
 SCM_API SCM scm_utime (SCM object, SCM actime, SCM modtime,
SCM actimens, SCM modtimens, SCM flags);
diff --git a/module/ice-9/spawn.scm b/module/ice-9/spawn.scm
new file mode 100644
index 0..ae4f54efa
--- /dev/null
+++ b/module/ice-9/spawn.scm
@@ -0,0 +1,54 @@
+;; Spawning programs
+
+ Copyright (C) 2022
+   Free Software Foundation, Inc.
+
+ This library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 3 of the License, or (at your option) any later version.
+
+ This library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with this library; if not, write to the Free Software
+ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 
USA
+
+
+(define-module (ice-9 spawn)
+  #:export (spawn))
+
+(define (port-with-defaults port default-mode)
+  (if (file-port? port)
+  port
+  (open-file "/dev/null" defau