>Number:         3673
>Category:       lib
>Synopsis:       popen can close the wrong file descriptors
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    lib-bug-people (Library Bug People)
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May 26 07:50:01 1997
>Last-Modified:
>Originator:     Dave Sainty
>Organization:
DTSP
>Release:        <NetBSD-current source date>
>Environment:
i386, NetBSD-current
System: NetBSD tequila.ext.nz 1.2E NetBSD 1.2E (TEQUILA) #21: Sun May 25 13:58:02 NZST 1997 dave@tequila.ext.nz:/vol/tequila/userC/NetBSD-current/src/sys/arch/i386/compile/TEQUILA i386


>Description:
	popen(3) does a dup2 before closing old files without checking if it's
	actually closing the file it dup2'd another file to.

>How-To-Repeat:
	By inspection, src/lib/libc/gen/popen.c.

        if (pdes[1] != STDOUT_FILENO) {
             (void)dup2(pdes[1], STDOUT_FILENO);
             (void)close(pdes[1]);
        }
        (void) close(pdes[0]);

	Consider if pdes[0] == STDOUT_FILENO.  pdes[1] is dup2'd to
	STDOUT_FILENO, but is then promptly closed.  Even worse, the POSIX
	code closes all current fd's open in the popen list, increasing the
	chances of death.

	The easy solution is to do all the closes first.  This is what the
	patch does.
>Fix:
--- /vol/src/NetBSD-current/src/lib/libc/gen/popen.c.orig	Tue Oct 15 04:38:36 1996
+++ /vol/src/NetBSD-current/src/lib/libc/gen/popen.c	Tue May 27 01:48:12 1997
@@ -91,26 +91,26 @@
 		return (NULL);
 		/* NOTREACHED */
 	case 0:				/* Child. */
+		/* POSIX.2 B.3.2.2 "popen() shall ensure that any streams
+		   from previous popen() calls that remain open in the 
+		   parent process are closed in the new child process. */
+		for (cur = pidlist; cur; cur = cur->next)
+			close(fileno(cur->fp));
+		
 		if (*type == 'r') {
+			(void) close(pdes[0]);
 			if (pdes[1] != STDOUT_FILENO) {
 				(void)dup2(pdes[1], STDOUT_FILENO);
 				(void)close(pdes[1]);
 			}
-			(void) close(pdes[0]);
 		} else {
+			(void)close(pdes[1]);
 			if (pdes[0] != STDIN_FILENO) {
 				(void)dup2(pdes[0], STDIN_FILENO);
 				(void)close(pdes[0]);
 			}
-			(void)close(pdes[1]);
 		}
 
-		/* POSIX.2 B.3.2.2 "popen() shall ensure that any streams
-		   from previous popen() calls that remain open in the 
-		   parent process are closed in the new child process. */
-		for (cur = pidlist; cur; cur = cur->next)
-			close(fileno(cur->fp));
-		
 		execl(_PATH_BSHELL, "sh", "-c", program, NULL);
 		_exit(127);
 		/* NOTREACHED */
>Audit-Trail:
>Unformatted: