open-pipe fd duplications

open-pipe fd duplications

[Kevin Ryde]

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

I think open-pipe leaves an extra copy of its input or output fd in
the forked child.  For instance, using showfds.c below and

	(use-modules (ice-9 popen))

	(let ((p (open-input-pipe "./showfds")))
	  (sleep 1)
	  (close-pipe p))

	(let ((p (open-output-pipe "./showfds")))
	  (sleep 1)
	  (close-pipe p))

I take it the intention is to leave the child clean except for the
normal 0, 1 and 2.

        * popen.scm (open-process): Close input-fdes, output-fdes and
        error-fdes after duping them to 0, 1 and 2.

I think this is worth putting in the 1.6 branch too.

The only likely ill effect I could spot from the current code was that
for an output pipe the extra copy of the read side means that if the
spawned program closes stdin, a write from the parent (ie. guile)
won't produce a SIGPIPE.  For instance the following contrivance
currently gets a SIGPIPE only when the sleep finishes and the shell
exits, whereas with the change above it happens immediately.

	(use-modules (ice-9 popen))
	(let ((p (open-output-pipe "exec 0>/dev/null; sleep 10")))
	  (while #t
	    (display "x" p)))



[-- Attachment #2: popen.scm.close.diff --]
[-- Type: text/plain, Size: 1087 bytes --]

--- popen.scm.~1.11.~	2003-04-07 08:04:58.000000000 +1000
+++ popen.scm	2003-07-27 14:08:21.000000000 +1000
@@ -1,6 +1,6 @@
 ;; popen emulation, for non-stdio based ports.
 
-;;;; Copyright (C) 1998, 1999, 2000, 2001 Free Software Foundation, Inc.
+;;;; Copyright (C) 1998, 1999, 2000, 2001, 2003 Free Software Foundation, Inc.
 ;;;; 
 ;;;; This library is free software; you can redistribute it and/or
 ;;;; modify it under the terms of the GNU Lesser General Public
@@ -90,14 +90,18 @@
 			  (set! output-fdes (dup->fdes 0)))
 		      (if (= error-fdes 0)
 			  (set! error-fdes (dup->fdes 0)))
-		      (dup2 input-fdes 0)))
+		      (dup2 input-fdes 0)
+		      (close-fdes input-fdes)))
 
 	       (cond ((not (= output-fdes 1))
 		      (if (= error-fdes 1)
 			  (set! error-fdes (dup->fdes 1)))
-		      (dup2 output-fdes 1)))
+		      (dup2 output-fdes 1)
+		      (close-fdes output-fdes)))
 
-	       (dup2 error-fdes 2)
+	       (cond ((not (= error-fdes 2))
+		      (dup2 error-fdes 2)
+		      (close-fdes error-fdes)))
 		     
 	       (apply execlp prog prog args)))
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: showfds.c --]
[-- Type: text/x-csrc, Size: 604 bytes --]

#include <fcntl.h>
#include <stdio.h>
#include <sys/stat.h>

int
main (void)
{
  int  i, found = 0;
  struct stat  st;
  fprintf (stderr, "file descriptors open:\n");

  for (i = 0; i < 20; i++)
    {
      if (fstat (i, &st) != -1)
        {
          found = 1;
          fprintf (stderr, "%d: %X %s\n",
                   i, st.st_mode,
                   S_ISREG (st.st_mode) ? "reg"
                   : S_ISCHR (st.st_mode) ? "chr"
                   : S_ISFIFO (st.st_mode) ? "fifo"
                   : "unknown");

        }
    }

  if (! found)
    fprintf (stderr, " none\n");

  return 0;
}

[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: open-pipe fd duplications

[Marius Vollmer]

Kevin Ryde <user42@zip.com.au> writes:

>         * popen.scm (open-process): Close input-fdes, output-fdes and
>         error-fdes after duping them to 0, 1 and 2.
>
> I think this is worth putting in the 1.6 branch too.

I agree.  Can you make that change?

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel

Re: open-pipe fd duplications

[Kevin Ryde]

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

Marius Vollmer <mvo@zagadka.de> writes:
>
> Can you make that change?

Yep, applied.

I realized I botched the first effort actually, didn't pay attention
to the comment ...

        * popen.scm (open-process): Correction to previous fdes closing
        change, need to watch out for stdin==stderr or stdout==stderr.

I added some tests, both of this and of the original problem.


[-- Attachment #2: popen.scm.close-2.diff --]
[-- Type: text/plain, Size: 1252 bytes --]

--- popen.scm.~1.12.~	1970-01-01 10:00:01.000000000 +1000
+++ popen.scm	2003-09-16 21:46:52.000000000 +1000
@@ -81,9 +81,8 @@
 						(= pt-fileno error-fdes)))
 				       (close-fdes pt-fileno))))))
 
-	       ;; copy the three selected descriptors to the standard
-	       ;; descriptors 0, 1, 2.  note that it's possible that
-	       ;; output-fdes or input-fdes is equal to error-fdes.
+	       ;; Copy the three selected descriptors to the standard
+	       ;; descriptors 0, 1, 2, if not already there
 
 	       (cond ((not (= input-fdes 0))
 		      (if (= output-fdes 0)
@@ -91,13 +90,17 @@
 		      (if (= error-fdes 0)
 			  (set! error-fdes (dup->fdes 0)))
 		      (dup2 input-fdes 0)
-		      (close-fdes input-fdes)))
-
+		      ;; it's possible input-fdes is error-fdes
+		      (if (not (= input-fdes error-fdes))
+			  (close-fdes input-fdes))))
+	       
 	       (cond ((not (= output-fdes 1))
 		      (if (= error-fdes 1)
 			  (set! error-fdes (dup->fdes 1)))
 		      (dup2 output-fdes 1)
-		      (close-fdes output-fdes)))
+		      ;; it's possible output-fdes is error-fdes
+		      (if (not (= output-fdes error-fdes))
+			  (close-fdes output-fdes))))
 
 	       (cond ((not (= error-fdes 2))
 		      (dup2 error-fdes 2)

[-- Attachment #3: popen.test --]
[-- Type: text/plain, Size: 4924 bytes --]

;;;; popen.test --- exercise ice-9/popen.scm      -*- scheme -*-
;;;;
;;;; Copyright 2003 Free Software Foundation, Inc.
;;;;
;;;; This library is free software; you can redistribute it and/or
;;;; modify it under the terms of the GNU Lesser General Public
;;;; License as published by the Free Software Foundation; either
;;;; version 2.1 of the License, or (at your option) any later version.
;;;; 
;;;; This library is distributed in the hope that it will be useful,
;;;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
;;;; Lesser General Public License for more details.
;;;; 
;;;; You should have received a copy of the GNU Lesser General Public
;;;; License along with this library; if not, write to the Free Software
;;;; Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA

(define-module (test-suite test-ice-9-popen)
  #:use-module (test-suite lib)
  #:use-module (ice-9 popen))


;; read from PORT until eof is reached, return what's read as a string
(define (read-string-to-eof port)
  (do ((lst '() (cons c lst))
       (c (read-char port) (read-char port)))
      ((eof-object? c)
       (list->string (reverse! lst)))))

;; call (THUNK), with SIGPIPE set to SIG_IGN so that an EPIPE error is
;; generated rather than a SIGPIPE signal
(define (with-epipe thunk)
  (dynamic-wind
      (lambda ()
	(sigaction SIGPIPE SIG_IGN))
      thunk
      restore-signals))


;;
;; open-input-pipe
;;

(with-test-prefix "open-input-pipe"
  
  (pass-if-exception "no args" exception:wrong-num-args
    (open-input-pipe))
  
  (pass-if "port?"
    (port? (open-input-pipe "echo hello")))
  
  (pass-if "echo hello"
    (string=? "hello\n" (read-string-to-eof (open-input-pipe "echo hello"))))
  
  ;; exercise file descriptor setups when stdin is the same as stderr  
  (pass-if "stdin==stderr"
    (let ((port (open-file "/dev/null" "r+")))
      (with-input-from-port port
	(lambda ()
	  (with-error-to-port port
	    (lambda ()
	      (open-input-pipe "echo hello"))))))
    #t)
  
  ;; exercise file descriptor setups when stdout is the same as stderr  
  (pass-if "stdout==stderr"
    (let ((port (open-file "/dev/null" "r+")))
      (with-output-to-port port
	(lambda ()
	  (with-error-to-port port
	    (lambda ()
	      (open-input-pipe "echo hello"))))))
    #t)
  
  ;; After the child closes stdout (which it indicates here by writing
  ;; "closed" to stderr), the parent should see eof.  In Guile 1.6.4 and
  ;; earlier a duplicate of stdout existed in the child, meaning eof was not
  ;; seen.
  (pass-if "no duplicate"
    (let* ((pair (pipe))
	   (port (with-error-to-port (cdr pair)
		   (lambda ()
		     (open-input-pipe
		      "exec 1>/dev/null; echo closed 1>&2; sleep 999")))))
      (read-char (car pair)) ;; wait for child to do its thing
      (and (char-ready? port)
	   (eof-object? (read-char port))))))

;;
;; open-output-pipe
;;

(with-test-prefix "open-output-pipe"
  
  (pass-if-exception "no args" exception:wrong-num-args
    (open-output-pipe))
  
  (pass-if "port?"
    (port? (open-output-pipe "exit 0")))
  
  ;; exercise file descriptor setups when stdout is the same as stderr  
  (pass-if "stdin==stderr"
    (let ((port (open-file "/dev/null" "r+")))
      (with-input-from-port port
	(lambda ()
	  (with-error-to-port port
	    (lambda ()
	      (open-output-pipe "exit 0"))))))
    #t)
  
  ;; exercise file descriptor setups when stdout is the same as stderr
  (pass-if "stdout==stderr"
    (let ((port (open-file "/dev/null" "r+")))
      (with-output-to-port port
	(lambda ()
	  (with-error-to-port port
	    (lambda ()
	      (open-output-pipe "exit 0"))))))
    #t)
  
  ;; After the child closes stdin (which it indicates here by writing
  ;; "closed" to stderr), the parent should see a broken pipe.  We setup to
  ;; see this as EPIPE (rather than SIGPIPE).  In Guile 1.6.4 and earlier a
  ;; duplicate of stdin existed in the child, preventing the broken pipe
  ;; occurring.
  (pass-if "no duplicate"
    (with-epipe
     (lambda ()
       (let* ((pair (pipe))
	      (port (with-error-to-port (cdr pair)
		      (lambda ()
			(open-output-pipe
			 "exec 0</dev/null; echo closed 1>&2; sleep 999")))))
	 (read-char (car pair)) ;; wait for child to do its thing
	 (catch 'system-error
		(lambda ()
		  (write-char #\x port)
		  (force-output port)
		  #f)
		(lambda (key name fmt args errno-list)
		  (= (car errno-list) EPIPE))))))))

;;
;; close-pipe
;;

(with-test-prefix "open-output-pipe"
  
  (pass-if-exception "no args" exception:wrong-num-args
    (close-pipe))
  
  (pass-if "exit 0"
    (let ((st (close-pipe (open-output-pipe "exit 0"))))
      (and (status:exit-val st)
	   (= 0 (status:exit-val st)))))
  
  (pass-if "exit 1"
    (let ((st (close-pipe (open-output-pipe "exit 1"))))
      (and (status:exit-val st)
	   (= 1 (status:exit-val st))))))


[-- Attachment #4: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://mail.gnu.org/mailman/listinfo/guile-devel