popen bug, extension

[Eric Blake <ebb9 at byu dot net>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

popen has a bug where it leaks fds to subsequent popen child processes if
fcntl is not supported, or if the parent explicitly messes with fcntl
after the fact.  Since it already maintains a list of popen'd fds in the
parent, we can exploit that list to obey POSIX without using FD_CLOEXEC in
the first place (and strace testing on recent BSD, Solaris, and glibc
hints that similar list management is being done in those implementations,
rather than use of FD_CLOEXEC).

Meanwhile, the use of FD_CLOEXEC is still nice if you plan on spawning
non-popen children (and have fcntl support), enough so that glibc supports
popen(cmd,"re") to create a stream with the cloexec bit already set.

And documentation is always nice.

OK to commit?  I have plans for an eventual followup patch to use pipe2 on
platforms that support that, so that a mode of "re" can atomically set the
cloexec bit in the parent (it still requires just as many syscalls, since
the child process then has to undo the cloexec bit on its side of the
pipe.  But using pipe2 would close a security hole where the parent's fd
can be leaked in a multi-threaded process if another thread does a
fork/exec between popen's use of pipe and vfork).  But no point
writing/testing that patch until pipe2 is more widely supported.

2009-08-18  Eric Blake  <ebb9@byu.net>

	Improve popen compatibility with glibc.
	* libc/posix/popen.c (popen): The 2006-08-22 change to use
	FD_CLOEXEC disagrees with other implementations; instead, use
	pidlist to work even when fcntl is not available.  Meanwhile,
	support the 'e' modifier to set CLOEXEC, as in glibc.  Drop
	cygwin-specific code, now that cygwin has its own version.
	* libc/posix/Makefile.am (CHEWOUT_FILES): Document popen.
	* libc/posix/posix.tex: New file.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             ebb9@byu.net
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqKy5kACgkQ84KuGfSFAYCo3wCfXkL+Jatgc/xmfzZLVybJEYVG
HGgAnj/LMpVRnLBtzrY4KBgWbLr1rYaB
=v7Og
-----END PGP SIGNATURE-----

? libc/autom4te.cache
Index: libc/posix/Makefile.am
===================================================================
RCS file: /cvs/src/src/newlib/libc/posix/Makefile.am,v
retrieving revision 1.11
diff -u -p -r1.11 Makefile.am
--- libc/posix/Makefile.am	24 Nov 2008 20:11:42 -0000	1.11
+++ libc/posix/Makefile.am	18 Aug 2009 15:40:02 -0000
@@ -51,7 +51,8 @@ endif # USE_LIBTOOL
 
 include $(srcdir)/../../Makefile.shared	
 
-CHEWOUT_FILES =
+CHEWOUT_FILES = \
+	popen.def
 
 SUFFIXES = .def
 
@@ -63,8 +64,8 @@ CHEW = ../../doc/makedoc -f $(srcdir)/..
 
 TARGETDOC = ../tmp.texi
 
-# No doc for posix.
-doc:
+doc: $(CHEWOUT_FILES)
+	cat $(srcdir)/posix.tex >> $(TARGETDOC)
 
 AM_CFLAGS = -D_GNU_SOURCE
 
Index: libc/posix/popen.c
===================================================================
RCS file: /cvs/src/src/newlib/libc/posix/popen.c,v
retrieving revision 1.7
diff -u -p -r1.7 popen.c
--- libc/posix/popen.c	31 Oct 2008 21:03:41 -0000	1.7
+++ libc/posix/popen.c	18 Aug 2009 15:40:02 -0000
@@ -32,6 +32,51 @@
  * SUCH DAMAGE.
  */
 
+/*
+FUNCTION
+<<popen>>, <<pclose>>---tie a stream to a command string
+
+INDEX
+	popen
+INDEX
+	pclose
+
+ANSI_SYNOPSIS
+	#include <stdio.h>
+	FILE *popen(char *<[s]>, char *<[mode]>);
+
+	int pclose(FILE *<[f]>);
+
+DESCRIPTION
+Use <<popen>> to create a stream to a child process executing a
+command string <<*<[s]>>> as processed by <</bin/sh>> on your system.
+The argument <[mode]> must start with either `<<r>>', where the stream
+reads from the child's <<stdout>>, or `<<w>>', where the stream wries
+to the child's <<stdin>>.  As an extension, <[mode]> may also contain
+`<<e>>' to set the close-on-exec bit of the parent's file descriptor.
+The stream created by <<popen>> must be closed by <<pclose>> to avoid
+resource leaks.
+
+Streams created by prior calls to <<popen>> are not visible in
+subsequent <<popen>> children, regardless of the close-on-exec bit.
+
+Use ``<<system(NULL)>>'' to test whether your system has <</bin/sh>>
+available.
+
+RETURNS
+<<popen>> returns a file stream opened with the specified <[mode]>,
+or <<NULL>> if a child process could not be created.  <<pclose>>
+returns 0 if the stream is successfully closed, or -1 if the stream
+was not created by <<popen>>.
+
+PORTABILITY
+POSIX.2 requires <<popen>> and <<pclose>>, but only specifies a mode
+of just <<r>> or <<w>>.  Where <<sh>> is found is left unspecified.
+
+Supporting OS subroutines required: <<_exit>>, <<_execve>>, <<_fork_r>>,
+<<_wait_r>>, <<pipe>>, <<fcntl>>, <<sbrk>>.
+*/
+
 #ifndef _NO_POPEN
 
 #if defined(LIBC_SCCS) && !defined(lint)
@@ -59,21 +104,21 @@ static struct pid {
 	struct pid *next;
 	FILE *fp;
 	pid_t pid;
-} *pidlist; 
-	
+} *pidlist;
+
 FILE *
 _DEFUN(popen, (program, type),
 	const char *program _AND
 	const char *type)
 {
-	struct pid *cur;
+	struct pid *cur, *last;
 	FILE *iop;
 	int pdes[2], pid;
 
        if ((*type != 'r' && *type != 'w')
 	   || (type[1]
-#ifdef __CYGWIN__
-	       && (type[2] || (type[1] != 'b' && type[1] != 't'))
+#ifdef HAVE_FCNTL
+	       && (type[2] || (type[1] != 'e'))
 #endif
 			       )) {
 		errno = EINVAL;
@@ -111,12 +156,11 @@ _DEFUN(popen, (program, type),
 			}
 			(void)close(pdes[1]);
 		}
+		/* Close all fd's created by prior popen.  */
+		for (last = NULL, cur = pidlist; cur;
+		     last = cur, cur = cur->next)
+			(void)close (fileno (cur->fp));
 		execl(_PATH_BSHELL, "sh", "-c", program, NULL);
-#ifdef __CYGWIN__
-		/* On cygwin32, we may not have /bin/sh.  In that
-                   case, try to find sh on PATH.  */
-		execlp("sh", "sh", "-c", program, NULL);
-#endif
 		_exit(127);
 		/* NOTREACHED */
 	}
@@ -131,9 +175,10 @@ _DEFUN(popen, (program, type),
 	}
 
 #ifdef HAVE_FCNTL
-	/* Hide pipe from future popens; assume fcntl can't fail.  */
-	fcntl (fileno (iop), F_SETFD,
-	       fcntl (fileno (iop), F_GETFD, 0) | FD_CLOEXEC);
+	/* Mark pipe cloexec if requested.  */
+	if (type[1] == 'e')
+		fcntl (fileno (iop), F_SETFD,
+		       fcntl (fileno (iop), F_GETFD, 0) | FD_CLOEXEC);
 #endif /* HAVE_FCNTL */
 
 	/* Link into list of file descriptors. */
@@ -177,7 +222,7 @@ _DEFUN(pclose, (iop),
 	else
 		last->next = cur->next;
 	free(cur);
-		
+
 	return (pid == -1 ? -1 : pstat);
 }
 
Index: libc/posix/posix.tex
===================================================================
RCS file: libc/posix/posix.tex
diff -N libc/posix/posix.tex
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ libc/posix/posix.tex	18 Aug 2009 15:40:02 -0000
@@ -0,0 +1,13 @@
+@node Posix
+@chapter Posix Functions
+
+This chapter groups several utility functions specified by POSIX, but
+not by C.  Each function documents which header to use.
+
+@menu
+* popen::       Create a stream tied to a child process
+@end menu
+
+@page
+@include posix/popen.def
+