unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
From: nathan via "Bug reports for GUILE, GNU's Ubiquitous Extension Language" <bug-guile@gnu.org>
To: 72092@debbugs.gnu.org, jakub-w@riseup.net
Subject: bug#72092: [PATCH] fix file descriptor leak in piped_process/system*/popen/etc
Date: Sat, 13 Jul 2024 18:22:58 +0000	[thread overview]
Message-ID: <0ea0f005-32be-2fa1-b96a-e67d3e4ec3f6@nborghese.com> (raw)
In-Reply-To: <77cbc4b2a48a74957cc02fcc53238b29@riseup.net>

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

fix is attached

test code:
(use-modules (ice-9 ftw))
(use-modules (ice-9 popen))
(define (print-open)
    (format #t "~a\n" (length (scandir "/proc/self/fd"))))
(let ((p (%make-void-port "rw")))
    (print-open)
    (parameterize ((current-output-port p) (current-input-port p) (current-error-port p))
      (system* "ls"))
    (close-port p)
    (print-open))
(parameterize ((current-error-port (%make-void-port OPEN_BOTH)))
      (while
       #t
       (close-pipe (open-pipe* OPEN_READ "free"))
       (print-open)
       (sleep 1)))

there's still a one-time increase in the file descriptors, but it seems unrelated. or at least much harder to find.

(by the way, ignoring current-output-port and using /dev/null is bad. it would be nice to open a pipe in that case and forward whatever the child writes to the scheme port.
but that would require a separate thread, and it may be impossible to do something like that for current-input-port)

[-- Attachment #2: 0001-fix-file-descriptor-leak-in-piped_process-system-pop.patch --]
[-- Type: text/x-patch, Size: 2936 bytes --]

From 466e4ab2c2cb1f93af481290c1d36db00976b8ce Mon Sep 17 00:00:00 2001
From: nathan <nathan_mail@nborghese.com>
Date: Sat, 13 Jul 2024 13:25:08 -0400
Subject: [PATCH] fix file descriptor leak in piped_process/system*/popen/etc

* libguile/posix.c (piped_process): close automatically opened /dev/null
  Fix bug #72092
---
 libguile/posix.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/libguile/posix.c b/libguile/posix.c
index 9a873b5a1..8028b493f 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -1535,32 +1535,32 @@ static int
 piped_process (pid_t *pid, SCM prog, SCM args, SCM from, SCM to)
 #define FUNC_NAME "piped-process"
 {
-  int reading, writing;
   int c2p[2]; /* Child to parent.  */
   int p2c[2]; /* Parent to child.  */
   int in = -1, out = -1, err = -1;
   char *exec_file;
   char **exec_argv;
   char **exec_env = environ;
+  int to_close[3];
+  int num_to_close=0;
 
   exec_file = scm_to_locale_string (prog);
   exec_argv = scm_i_allocate_string_pointers (scm_cons (prog, args));
 
-  reading = scm_is_pair (from);
-  writing = scm_is_pair (to);
-
-  if (reading)
+  if (scm_is_pair (from))
     {
       c2p[0] = scm_to_int (scm_car (from));
       c2p[1] = scm_to_int (scm_cdr (from));
       out = c2p[1];
+      to_close[num_to_close++] = out;
     }
 
-  if (writing)
+  if (scm_is_pair (to))
     {
       p2c[0] = scm_to_int (scm_car (to));
       p2c[1] = scm_to_int (scm_cdr (to));
       in = p2c[0];
+      to_close[num_to_close++] = in;
     }
 
   {
@@ -1569,30 +1569,37 @@ piped_process (pid_t *pid, SCM prog, SCM args, SCM from, SCM to)
     if (SCM_OPOUTFPORTP ((port = scm_current_error_port ())))
       err = SCM_FPORT_FDES (port);
     else
-      err = open ("/dev/null", O_WRONLY | O_CLOEXEC);
+      {
+        err = open ("/dev/null", O_WRONLY | O_CLOEXEC);
+        to_close[num_to_close++] = err;
+      }
     if (out == -1)
       {
         if (SCM_OPOUTFPORTP ((port = scm_current_output_port ())))
           out = SCM_FPORT_FDES (port);
         else
-          out = open ("/dev/null", O_WRONLY | O_CLOEXEC);
+          {
+            out = open ("/dev/null", O_WRONLY | O_CLOEXEC);
+            to_close[num_to_close++] = out;
+          }
       }
     if (in == -1)
       {
         if (SCM_OPINFPORTP ((port = scm_current_input_port ())))
           in = SCM_FPORT_FDES (port);
         else
-          in = open ("/dev/null", O_RDONLY | O_CLOEXEC);
+          {
+            in = open ("/dev/null", O_RDONLY | O_CLOEXEC);
+            to_close[num_to_close++] = in;
+          }
       }
   }
 
   *pid = do_spawn (exec_file, exec_argv, exec_env, in, out, err, 1);
   int errno_save = (*pid < 0) ? errno : 0;
 
-  if (reading)
-    close (c2p[1]);
-  if (writing)
-    close (p2c[0]);
+  while (num_to_close>0)
+    close (to_close[--num_to_close]);
 
   if (*pid == -1)
     switch (errno_save)
-- 
2.45.2


      reply	other threads:[~2024-07-13 18:22 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-07-13 12:07 bug#72092: popen creates "/dev/null" fds and never closes them jakub-w
2024-07-13 18:22 ` nathan via Bug reports for GUILE, GNU's Ubiquitous Extension Language [this message]

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=0ea0f005-32be-2fa1-b96a-e67d3e4ec3f6@nborghese.com \
    --to=bug-guile@gnu.org \
    --cc=72092@debbugs.gnu.org \
    --cc=jakub-w@riseup.net \
    --cc=nathan_mail@nborghese.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).