bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely.
* libguile/posix.c (renumber_file_descriptor): Refactor it as
dup_handle_error.
(dup_handle_error, dup2_handle_error): New functions that wrap around
dup and dup2 by retrying on EINTR or EBUSY, as well as erroring out on
other errors.
(start_child): Close standard file descriptors only after all of them
have been dup2'd.
---
libguile/posix.c | 84 +++-
1 file changed, 54 insertions(+), 30 deletions(-)
diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..e9f49fa27 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1280,14 +1280,14 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#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. */
+/* 'dup_handle_error' 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, does *not* close 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)
+dup_handle_error (int fd, int err)
{
int new_fd;
@@ -1304,7 +1304,33 @@ renumber_file_descriptor (int fd, int err)
_exit (127); /* Use exit status 127, as with other exec errors. */
}
- close (fd);
+ return new_fd;
+}
+
+/* 'dup2_handle_error' 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 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. */
+static int
+dup2_handle_error (int fd, int to, int err)
+{
+ int new_fd;
+
+ do
+new_fd = dup2 (fd, to);
+ while (new_fd == -1 && (errno == EINTR || errno == EBUSY));
+
+ 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. */
+}
+
return new_fd;
}
#endif /* HAVE_FORK */
@@ -1357,34 +1383,32 @@ start_child (const char *exec_file, char **exec_argv,
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);
- close (out);
-}
- if (err > 2)
-{
- do dup2 (err, 2); while (errno == EINTR);
- close (err);
-}
+ /* Dup each non-yet-dup2'd fd that's in the way to the next available fd,
+ so that we can safely dup2 to 0/1/2 without potentially overwriting
+ in/out/err. Note that dup2 doesn't do anything if its arguments are
+ equal. */
+ if (out == 0)
+out = dup_handle_error (out, err);
+ if (err == 0)
+err = dup_handle_error (err, err);
+ dup2_handle_error (in, 0, err);
+
+ if (err == 1)
+err = dup_handle_error (err, err);
+ dup2_handle_error (out, 1, err);
+
+ dup2_handle_error (err, 2, err);
+
+ if (in > 2) close (in);
+ if (out > 2) close (out);
+ if (err > 2) 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",
+dprintf (2, "In execvp of %s: %s\n",
exec_file, msg);
}
--
2.36.0
bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process.
* libguile/posix.c (scm_piped_process): Avoid double closes.
---
libguile/posix.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/libguile/posix.c b/libguile/posix.c
index e9f49fa27..155ad09b7 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1475,12 +1475,18 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
if (reading)
{
close (c2p[0]);
- close (c2p[1]);
+ if (c2p[1] != c2p[0])
+close (c2p[1]);
}
if (writing)
{
- close (p2c[0]);
- close (p2c[1]);
+ if (!(reading && (c2p[0] == p2c[0] ||
+c2p[1] == p2c[0])))
+close (p2c[0]);
+ if (p2c[0] != p2c[1] &&
+ !(reading && (c2p[0] == p2c[1] ||
+c2p[1] == p2c[1])))
+close (p2c[1]);
}
errno = errno_save;
SCM_SYSERROR;
@@ -1488,7 +1494,7 @@ scm_piped_process (SCM prog, SCM args, SCM from, SCM to)
if (reading)
close (c2p[1]);
- if (writing)
+ if (writing && !(reading && c2p[1] == p2c[0]))
close (p2c[0]);
return scm_from_int (pid);
--
2.36.0
bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child.
* libguile/posix.c (start_child): We're closing all fds anyway, no need to try to close some specific ones beforehand. --- libguile/posix.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/libguile/posix.c b/libguile/posix.c index 155ad09b7..94a043cca 100644 --- a/libguile/posix.c +++ b/libguile/posix.c @@ -1363,12 +1363,6 @@ start_child (const char *exec_file, char **exec_argv, 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--) -- 2.36.0
bug#52835: [PATCH v4 4/4] Make start_child propagate the child errno to the parent.
* 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 ]])
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, l
bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process.
retitle 52835 Improve safety of start_child and piped-process. thanks Hello everyone, This time, it's another Guix bug [1] that prompted me to have a closer look at piped-process and start_child, which don't seem to be very multi-thread safe. I've ended up with a couple of improvements that IMO would make all procedures relying on them more robust. Here's roughly what I did: * Fix the fd closing code that was bogus for unusual values for in, out, err for start_child. * Check for double closes and avoid them, so that we don't accidentally close an fd that another thread could have opened. * Remove some closing code in the child, since we're already generically closing all fds. * Add a pipe from the child to the parent that the former uses to report its errno to the latter. This avoids the use of strerror and printf in the child after forking, since they are not async-signal safe. As a side effect, this lets piped-error raise the proper system exception for the child errno, instead of returning the PID of a process that hasn't exec'd successfully. [1] https://issues.guix.gnu.org/55441 Best, Josselin Poiret (4): Fix child spawning closing standard fds prematurely. Avoid double closes in piped-process. Remove useless closing code in start_child. Make start_child propagate the child errno to the parent. configure.ac | 3 +- libguile/posix.c | 187 ++- 2 files changed, 138 insertions(+), 52 deletions(-) -- 2.36.0
