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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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
  0 siblings, 0 replies; 6+ 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] 6+ messages in thread

end of thread, other threads:[~2021-12-28 17:25 UTC | newest]

Thread overview: 6+ 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

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