unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
* bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
@ 2021-12-27 21:25 Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:35 ` bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2021-12-27 21:25 UTC (permalink / raw)
  To: 52835; +Cc: Josselin Poiret

Hello,

While working on the Guix installer (that needs to use system* quite a
lot), I've noticed that output/error redirection doesn't behave as
intended when combined with system*.  Here's a test you can try
at home:
--8<---------------cut here---------------start------------->8---
(call-with-output-file "/tmp/test.log"
  (lambda (port) (with-output-to-port port
                   (lambda () (with-error-to-port port
                                (lambda () (system* "bash" "-c" "echo bong >&2")))))))
--8<---------------cut here---------------end--------------->8---

With current Guix, you will notice that /tmp/test.log is empty,
instead of the expected "bong".

Worse even, when testing with
--8<---------------cut here---------------start------------->8---
(with-error-to-port (current-output-port) (lambda () (system* "bash" "-c" "echo $$; sleep 10")))
--8<---------------cut here---------------end--------------->8---
you can actually inspect `/proc/<PID>/fd/` and see that the stderr fd,
2, is actually closed.  This means that the next opened fd will take
its place, to which writes to stderr may end up.

The logic behind the stdin/out/err redirection for child processes
lies in `start_child`, in libguile/posix.c, and doesn't take into
account cases like:
* in/out/err having common values, as the common fd will be closed
before it has been dup2'd to all the std fds (which happens in the
first example);
* in/out/err having values between 0 and 2 which aren't their
corresponding std fd number, as they will not be dup2'd to
the stdin/out/err (which happens in the second example).

The first patch addresses this by:
* moving in/out/err closing logic after they've all been dup2'd;
* removing the check that in/out/err are > the corresponding
stdin/out/err;
* replacing renumber_file_descriptor by simply dup, as the former
closes fds that might be shared.  The closing logic of the first point
is enough here.

The second patch removes renumber_file_descriptor, as it is no longer
used.

Best,
Josselin

Josselin Poiret (2):
  Fix child spawning closing standard fds prematurely
  Remove unused renumber_file_descriptor

 libguile/posix.c | 72 ++++++++++++++----------------------------------
 1 file changed, 20 insertions(+), 52 deletions(-)

-- 
2.34.0






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely
  2021-12-27 21:25 bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2021-12-27 21:35 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:35 ` bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-28 15:40 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Timothy Sample
  2 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2021-12-27 21:35 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 52835

* libguile/posix.c (start_child): Close standard file descriptors only
after all of them have been dup2'd.
---
 libguile/posix.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..148ebeb3d 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1357,27 +1357,25 @@ 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)
+    do out = dup (out); while (errno == EINTR);
+  if (err == 0)
+    do err = dup (err); while (errno == EINTR);
+  do dup2 (in, 0); while (errno == EINTR);
+
+  if (err == 1)
+    do err = dup (err); while (errno == EINTR);
+  do dup2 (out, 1); while (errno == EINTR);
+
+  do dup2 (err, 2); while (errno == EINTR);
+
+  if (in > 2) close (in);
+  if (out > 2) close (out);
+  if (err > 2) close (err);
 
   execvp (exec_file, exec_argv);
 
-- 
2.34.0






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor
  2021-12-27 21:25 bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:35 ` bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2021-12-27 21:35 ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:49   ` bug#52835: [PATCH v2 " Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-28 15:40 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Timothy Sample
  2 siblings, 1 reply; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2021-12-27 21:35 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 52835

* libguile/posix.c (renumber_file_descriptor): Remove it.
---
 libguile/posix.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 148ebeb3d..2624d07c2 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1277,37 +1277,7 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
   return scm_from_int (pid);
 }
 #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 */
+#endif /* HAVE_FORK *
 
 #ifdef HAVE_FORK
 #define HAVE_START_CHILD 1
-- 
2.34.0






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v2 2/2] Remove unused renumber_file_descriptor
  2021-12-27 21:35 ` bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2021-12-27 21:49   ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  0 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2021-12-27 21:49 UTC (permalink / raw)
  To: Josselin Poiret; +Cc: 52835

* libguile/posix.c (renumber_file_descriptor): Remove it.
---
Sorry for the noise, but I just saw that this patch omitted a closing
*/, here is a fixed version.

 libguile/posix.c | 30 ------------------------------
 1 file changed, 30 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 148ebeb3d..1d9996565 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1279,36 +1279,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
-- 
2.34.0






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
  2021-12-27 21:25 bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:35 ` bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2021-12-27 21:35 ` bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2021-12-28 15:40 ` Timothy Sample
  2021-12-28 17:25   ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2 siblings, 1 reply; 12+ messages in thread
From: Timothy Sample @ 2021-12-28 15:40 UTC (permalink / raw)
  To: 52835; +Cc: dev

Hey Josselin,

Thanks for finding this bug!  I have one concern about your patches:

Josselin Poiret writes:

> The second patch removes renumber_file_descriptor, as it is no longer
> used.

One thing that ‘renumber_file_descriptor’ does that we seem to be losing
here is error checking.  To my eye, the old code will try and warn the
user if they run out of file descriptors, but the new code will not.

The other thing that I like is how ‘renumber_file_descriptor’ checks the
return value of ‘dup’ in addition to checking ‘errno’.  (I realize that
the old code skips that check for ‘dup2’ – I’m kinda just stating a
preference here.)  A quick check of POSIX turns up the following: “the
value of ‘errno’ should only be examined when it is indicated to be
valid by a function’s return value” [1].


-- Tim

[1] https://pubs.opengroup.org/onlinepubs/9699919799/





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly
  2021-12-28 15:40 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Timothy Sample
@ 2021-12-28 17:25   ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-02-07 16:55     ` bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  0 siblings, 1 reply; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2021-12-28 17:25 UTC (permalink / raw)
  To: Timothy Sample; +Cc: 52835

Hello Timothy,

Timothy Sample <samplet@ngyro.com> writes:

> One thing that ‘renumber_file_descriptor’ does that we seem to be losing
> here is error checking.  To my eye, the old code will try and warn the
> user if they run out of file descriptors, but the new code will not.

I may have been too hasty on that front, you're right that we should
check for errors when dup'ing.  Note though that we should not run out
of fds, as we close all of them except for in/out/err right before using
it, but still, here are the interesting error codes:
* EINTR, although OpenBSD and Linux will never set that error from my
light research;
* EBUSY, not POSIX though, only Linux, so should we?
* EMFILE, if we run out of fds;
* EBADF if either of the fds are invalid.

As for error reporting, should we keep the parent's stderr fd open while
we do all of this, just to be able to report an error in case it
happens?  The old code used to try reporting the errors to the new `err`
fd instead, but it might be clearer to use stderr.

WDYT?

-- 
Josselin Poiret





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely
  2021-12-28 17:25   ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-02-07 16:55     ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  0 siblings, 1 reply; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-02-07 16:55 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

* 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.
---
Hello,

This is a new version of the fix that should now handle possible
dup/dup2 errors properly.

Best,
Josselin
 libguile/posix.c | 82 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 29 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 3ab12b99e..dc3080b3c 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,27 +1383,25 @@ 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);
 
-- 
2.34.0






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process.
  2022-02-07 16:55     ` bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-05-28 12:46       ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
                           ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-05-28 12:46 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

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






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely.
  2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-05-28 12:46         ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-05-28 12:46 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

* 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






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process.
  2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-05-28 12:46         ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` 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
  3 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-05-28 12:46 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

* 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






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child.
  2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-05-28 12:46         ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  2022-05-28 12:46         ` 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
  3 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-05-28 12:46 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

* 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






^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#52835: [PATCH v4 4/4] Make start_child propagate the child errno to the parent.
  2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
                           ` (2 preceding siblings ...)
  2022-05-28 12:46         ` bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
@ 2022-05-28 12:46         ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
  3 siblings, 0 replies; 12+ messages in thread
From: Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language @ 2022-05-28 12:46 UTC (permalink / raw)
  To: Josselin Poiret, Timothy Sample; +Cc: 52835

* 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






^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2022-05-28 12:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-27 21:25 bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:35 ` bug#52835: [PATCH 1/2] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:35 ` bug#52835: [PATCH 2/2] Remove unused renumber_file_descriptor Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-27 21:49   ` bug#52835: [PATCH v2 " Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2021-12-28 15:40 ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Timothy Sample
2021-12-28 17:25   ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-02-07 16:55     ` bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46       ` bug#52835: [PATCH v4 0/4] Improve safety of start_child and piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46         ` bug#52835: [PATCH v4 1/4] Fix child spawning closing standard fds prematurely Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46         ` bug#52835: [PATCH v4 2/4] Avoid double closes in piped-process Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46         ` bug#52835: [PATCH v4 3/4] Remove useless closing code in start_child Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-05-28 12:46         ` 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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).