unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
From: Josselin Poiret via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" <bug-guile@gnu.org>
To: Josselin Poiret <dev@jpoiret.xyz>, Timothy Sample <samplet@ngyro.com>
Cc: 52835@debbugs.gnu.org
Subject: bug#52835: [PATCH v3] Fix child spawning closing standard fds prematurely
Date: Mon,  7 Feb 2022 17:55:43 +0100	[thread overview]
Message-ID: <20220207165543.12723-1-dev@jpoiret.xyz> (raw)
In-Reply-To: <87lf04fxqo.fsf@jpoiret.xyz>

* 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






  reply	other threads:[~2022-02-07 16:55 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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     ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language [this message]
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
2022-09-05  6:48         ` bug#52835: [PATCH v5 0/3] Move spawning procedures to posix_spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05  6:48           ` bug#52835: [PATCH v5 1/3] Update gnulib to 0.1.5414-8204d and add posix_spawn, posix_spawnp Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05  6:48           ` bug#52835: [PATCH v5 2/3] Add spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-09-05  6:48           ` bug#52835: [PATCH v5 3/3] Move popen and posix procedures to spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-11-29 15:05             ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-11 20:16               ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-12 23:49                 ` Ludovic Courtès
2022-12-22 12:49                   ` bug#52835: [PATCH v6 0/3] Move spawning procedures to posix_spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49                     ` bug#52835: [PATCH v6 1/3] Add spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49                     ` bug#52835: [PATCH v6 2/3] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-22 12:49                     ` bug#52835: [PATCH v6 3/3] Move popen and posix procedures to spawn* Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 10:53                     ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-23 17:15                       ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 17:17                         ` bug#52835: [PATCH v7 1/2] Add spawn* and spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-23 17:17                           ` bug#52835: [PATCH v7 2/2] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2022-12-25 17:04                             ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2022-12-25 17:03                           ` Ludovic Courtès
2022-12-25 16:58                         ` Ludovic Courtès
2023-01-07 16:07                           ` Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-07 16:07                             ` bug#52835: [PATCH v8 1/2] Add spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-07 16:07                               ` bug#52835: [PATCH v8 2/2] Make system* and piped-process internally use spawn Josselin Poiret via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-12 22:02                             ` bug#52835: [PATCH 0/2] Fix spawning a child not setting standard fds properly Ludovic Courtès
2023-01-13  1:11 ` Andrew Whatson via Bug reports for GUILE, GNU's Ubiquitous Extension Language
2023-01-13 15:20   ` Ludovic Courtès

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220207165543.12723-1-dev@jpoiret.xyz \
    --to=bug-guile@gnu.org \
    --cc=52835@debbugs.gnu.org \
    --cc=dev@jpoiret.xyz \
    --cc=samplet@ngyro.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).