bug#52835: [PATCH v4 4/4] Make start_child propagate the child errno to the parent.

[Josselin Poiret via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" <bug-guile@gnu.org>]

* configure.ac: Add AC_CHECK_FUNCS for pipe2.
* libguile/posix.c (start_child): Use a pipe to transmit the child's
errno to the parent, which can then use it to signal an error instead of
writing to its error file descriptor.  This also avoids the use of
async-signal unsafe functions inside the child before exec.  Use pipe2
when available.
(dup_handle_error,dup2_handle_error): Ditto.
---
 configure.ac     |   3 +-
 libguile/posix.c | 113 ++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 89 insertions(+), 27 deletions(-)

diff --git a/configure.ac b/configure.ac
index 827e1c09d..19441a52e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -525,6 +525,7 @@ AC_CHECK_HEADERS([assert.h crt_externs.h])
 #   fork - unavailable on Windows
 #   sched_getaffinity, sched_setaffinity - GNU extensions (glibc)
 #   sendfile - non-POSIX, found in glibc
+#   pipe2 - non-POSIX, found on Linux
 #
 AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid         \
   fesetround ftime ftruncate fchown fchmod getcwd geteuid getsid        \
@@ -536,7 +537,7 @@ AC_CHECK_FUNCS([DINFINITY DQNAN cexp chsize clog clog10 ctermid         \
   getgrent kill getppid getpgrp fork setitimer getitimer strchr strcmp  \
   index bcopy memcpy rindex truncate isblank _NSGetEnviron              \
   strcoll strcoll_l strtod_l strtol_l newlocale uselocale utimensat     \
-  sched_getaffinity sched_setaffinity sendfile])
+  sched_getaffinity sched_setaffinity sendfile pipe2])
 
 # The newlib C library uses _NL_ prefixed locale langinfo constants.
 AC_CHECK_DECLS([_NL_NUMERIC_GROUPING], [], [], [[#include <langinfo.h>]])
diff --git a/libguile/posix.c b/libguile/posix.c
index 94a043cca..79f097756 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1284,8 +1284,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
    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, does *not* close the original FD, and returns
-   the new descriptor.  If dup(2) fails, print an error message to ERR
-   and abort.  */
+   the new descriptor.  If dup(2) fails, send errno to ERR and abort.  */
 static int
 dup_handle_error (int fd, int err)
 {
@@ -1299,9 +1298,12 @@ dup_handle_error (int fd, int err)
     {
       /* 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. */
+      int errno_save = errno;
+      while (write (err, &errno_save, sizeof (errno)) == -1)
+        if (errno != EINTR)
+          break;
+
+      _exit (127);
     }
 
   return new_fd;
@@ -1311,8 +1313,8 @@ dup_handle_error (int fd, int err)
    is specialized for that particular environment where it doesn't make
    sense to report errors via exceptions.  It uses dup2(2) to duplicate
    the file descriptor FD, does *not* close the original FD, and returns
-   the new descriptor.  If dup2(2) fails, print an error message to ERR
-   and abort.  */
+   the new descriptor.  If dup2(2) fails, send errno to ERR and
+   abort.  */
 static int
 dup2_handle_error (int fd, int to, int err)
 {
@@ -1326,9 +1328,11 @@ dup2_handle_error (int fd, int to, int err)
     {
       /* 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. */
+      int errno_save = errno;
+      while (write (err, &errno_save, sizeof (errno)) == -1)
+        if (errno != EINTR)
+          break;
+      _exit (127);
     }
 
   return new_fd;
@@ -1347,6 +1351,8 @@ start_child (const char *exec_file, char **exec_argv,
 {
   int pid;
   int max_fd = 1024;
+  int errno_save;
+  int errpipefds[2];
 
 #if defined (HAVE_GETRLIMIT) && defined (RLIMIT_NOFILE)
   {
@@ -1356,17 +1362,73 @@ start_child (const char *exec_file, char **exec_argv,
   }
 #endif
 
+/* Setup a pipe to receive the errno of the child, which can't call
+   async-signal unsafe functions such as strerror or the printf family. */
+#ifdef HAVE_PIPE2
+  if (pipe2 (errpipefds, O_CLOEXEC))
+    return -1;
+#else
+  if (pipe (errpipefds))
+    return -1;
+  /* Set the writer end as close-on-exec, so that it automatically
+     closes on successful exec, and so that other threads that fork+exec
+     will not block it.
+
+     XXX: There can potentially be a race issue between the pipe and
+     fcntl calls, such that another thread that forks in between
+     inherits the fd without CLOEXEC.  There is no POSIXy way to make
+     the combination atomic, so just hope that any other fork would
+     release resources it doesn't need, like we do. */
+  if (fcntl (errpipefds[1], F_SETFD, fcntl (errpipefds[1], F_GETFD) | FD_CLOEXEC))
+    {
+      errno_save = errno;
+      close (errpipefds[0]);
+      close (errpipefds[1]);
+      errno = errno_save;
+      return -1;
+    }
+#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;
+    {
+      /* We're in the parent process. */
+      int read_count;
+      close (errpipefds[1]);
+      if (pid == -1)
+        {
+          /* Fork failed. */
+          errno_save = errno;
+          close (errpipefds[0]);
+          errno = errno_save;
+          return -1;
+        }
+
+      /* Fork successful, try to read a potential child errno from the pipe. */
+      while ((read_count = read(errpipefds[0], &errno_save, sizeof (errno))) == -1)
+        if (errno != EAGAIN && errno != EINTR)
+          break;
+      if (read_count == -1)
+        errno_save = errno;
+      close (errpipefds[0]);
+      if (read_count != 0)
+        {
+          /* Either the read failed (-1) or the child sent us an errno (> 0) */
+          errno = errno_save;
+          return -1;
+        }
+      return pid;
+    }
+
+  /* From now on, we are single threaded because of fork, so double
+     closes should be a no-op. */
 
   /* Close all file descriptors in ports inherited from the parent
-     except for in, out, and err.  Heavy-handed, but robust.  */
+     except for in, out, err and the error pipe back to the parent.
+     Heavy-handed, but robust.  */
   while (max_fd--)
-    if (max_fd != in && max_fd != out && max_fd != err)
+    if (max_fd != in && max_fd != out && max_fd != err && max_fd != errpipefds[1])
       close (max_fd);
 
   /* Ignore errors on these open() calls.  */
@@ -1382,16 +1444,16 @@ start_child (const char *exec_file, char **exec_argv,
      in/out/err.  Note that dup2 doesn't do anything if its arguments are
      equal. */
   if (out == 0)
-    out = dup_handle_error (out, err);
+    out = dup_handle_error (out, errpipefds[1]);
   if (err == 0)
-    err = dup_handle_error (err, err);
-  dup2_handle_error (in, 0, err);
+    err = dup_handle_error (err, errpipefds[1]);
+  dup2_handle_error (in, 0, errpipefds[1]);
 
   if (err == 1)
-    err = dup_handle_error (err, err);
-  dup2_handle_error (out, 1, err);
+    err = dup_handle_error (err, errpipefds[1]);
+  dup2_handle_error (out, 1, errpipefds[1]);
 
-  dup2_handle_error (err, 2, err);
+  dup2_handle_error (err, 2, errpipefds[1]);
 
   if (in > 2) close (in);
   if (out > 2) close (out);
@@ -1400,11 +1462,10 @@ start_child (const char *exec_file, char **exec_argv,
   execvp (exec_file, exec_argv);
 
   /* The exec failed!  There is nothing sensible to do.  */
-  {
-    const char *msg = strerror (errno);
-    dprintf (2, "In execvp of %s: %s\n",
-             exec_file, msg);
-  }
+  errno_save = errno;
+  while (write (errpipefds[1], &errno_save, sizeof (errno)) == -1)
+    if (errno != EINTR)
+      break;
 
   /* Use exit status 127, like shells in this case, as per POSIX
      <http://pubs.opengroup.org/onlinepubs/007904875/utilities/xcu_chap02.html#tag_02_09_01_01>.  */
-- 
2.36.0