bug#72092: popen creates "/dev/null" fds and never closes them

bug#72092: popen creates "/dev/null" fds and never closes them

[jakub-w]

Consider the following example:

(use-modules (ice-9 popen))

(parameterize ((current-error-port (%make-void-port OPEN_BOTH)))
  (while #t
    (close-pipe (open-pipe* OPEN_READ "free"))
    (sleep 1)))

This opens a new "/dev/null" file descriptor every second and doesn't
close it when close-pipe is called.
If current ports are all fd ports, this doesn't happen.

AFAICT the problem was introduced by the commit
36fd2b4920ae926c79b936c29e739e71a6dff2bc in Guile 3.0.10.

bug#72092: [PATCH] fix file descriptor leak in piped_process/system*/popen/etc

[nathan via Bug reports for GUILE, GNU's Ubiquitous Extension Language]

[-- 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