--4605703-10461-1063404607=:2576
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
Content-Disposition: INLINE

On 12 Sep 2003 16:41:58 -0400 "Nathan J. Williams" <nathanw@wasabisystems.com> wrote:
Nathan J. Williams <nathanw@wasabisystems.com> writes:
> Christian Limpach <chris@pin.lu> writes:
> 
> > The appended patch should make popen()/pclose() reentrant.  It adds a
> > lockfile for access to the pidlist and it prevents concurrent pclose/fclose
> > calls on the descriptors in the pidlist from interfering with the child
> > closing those descriptors.
> 
> (Man, I had no idea that popen()/pclose() went through all these
> contortions. Yuck.)

yes :-(

> Why is this using a rwlock instead of a mutex, since it never uses the
> lock in read-only mode?

I was using the lock in read-only mode at first before I added the caching
of file descriptors.

> What's the reason for caching the file descriptor numbers of all pipes
> in the pidlist? It's okay to use fileno(); FLOCKFILE()/FUNLOCKFILE()
> are recursive locks.

well, the problem is that the child is going to lose if it calls fileno()
on a file descriptor which is locked in the parent.  The child's
fileno/FLOCKFILE call will call cond_wait() and this will (try to) switch
to a different thread.  I've observed this exact failure case.

Appended is a new patch which is simpler:
- caches the file descriptor at creation time.
- doesn't hold the locks on the file streams during the vfork.

This is more like what I had at first, before I got overzealous and thought
it would be necessary to prevent an fclose() on a FILE* returned by popen()
from interfering with the child's closing of the same file handles.  This
is not necessary since the only close operation supported on FILE* returned
by popen() is pclose() and this is handled by the pidlist lock.

I have left the file descriptor caching conditional on _REENTRANT, it saves
a few bytes in the !_REENTRANT case.  I don't know if it's worth it, but
making it conditional is conservative.

Thank you for your comments.


--4605703-10461-1063404607=:2576
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII; NAME="popen-reentrant2.txt"
Content-Disposition: INLINE; FILENAME="popen-reentrant2.txt"

Index: lib/libc/gen/popen.c
===================================================================
RCS file: /cvs/netbsd/src/lib/libc/gen/popen.c,v
retrieving revision 1.27
diff -u -r1.27 popen.c
--- lib/libc/gen/popen.c	7 Aug 2003 16:42:55 -0000	1.27
+++ lib/libc/gen/popen.c	12 Sep 2003 22:06:57 -0000
@@ -68,9 +68,16 @@
 static struct pid {
 	struct pid *next;
 	FILE *fp;
+#ifdef _REENTRANT
+	int fd;
+#endif
 	pid_t pid;
 } *pidlist; 
 	
+#ifdef _REENTRANT
+static rwlock_t pidlist_lock = RWLOCK_INITIALIZER;
+#endif
+
 FILE *
 popen(command, type)
 	const char *command, *type;
@@ -108,11 +115,13 @@
 		return (NULL);
 	}
 
+	rwlock_rdlock(&pidlist_lock);
 	rwlock_rdlock(&__environ_lock);
 	switch (pid = vfork()) {
 	case -1:			/* Error. */
 		serrno = errno;
 		rwlock_unlock(&__environ_lock);
+		rwlock_unlock(&pidlist_lock);
 		free(cur);
 		(void)close(pdes[0]);
 		(void)close(pdes[1]);
@@ -124,7 +133,11 @@
 		   from previous popen() calls that remain open in the 
 		   parent process are closed in the new child process. */
 		for (old = pidlist; old; old = old->next)
+#ifdef _REENTRANT
+			close(old->fd); /* don't allow a flush */
+#else
 			close(fileno(old->fp)); /* don't allow a flush */
+#endif
 
 		if (*type == 'r') {
 			(void)close(pdes[0]);
@@ -151,9 +164,15 @@
 	/* Parent; assume fdopen can't fail. */
 	if (*type == 'r') {
 		iop = fdopen(pdes[0], type);
+#ifdef _REENTRANT
+		cur->fd = pdes[0];
+#endif
 		(void)close(pdes[1]);
 	} else {
 		iop = fdopen(pdes[1], type);
+#ifdef _REENTRANT
+		cur->fd = pdes[1];
+#endif
 		(void)close(pdes[0]);
 	}
 
@@ -162,6 +181,7 @@
 	cur->pid =  pid;
 	cur->next = pidlist;
 	pidlist = cur;
+	rwlock_unlock(&pidlist_lock);
 
 	return (iop);
 }
@@ -181,25 +201,32 @@
 
 	_DIAGASSERT(iop != NULL);
 
+	rwlock_wrlock(&pidlist_lock);
+
 	/* Find the appropriate file pointer. */
 	for (last = NULL, cur = pidlist; cur; last = cur, cur = cur->next)
 		if (cur->fp == iop)
 			break;
-	if (cur == NULL)
+	if (cur == NULL) {
+		rwlock_unlock(&pidlist_lock);
 		return (-1);
+	}
 
 	(void)fclose(iop);
 
-	do {
-		pid = waitpid(cur->pid, &pstat, 0);
-	} while (pid == -1 && errno == EINTR);
-
 	/* Remove the entry from the linked list. */
 	if (last == NULL)
 		pidlist = cur->next;
 	else
 		last->next = cur->next;
+
+	rwlock_unlock(&pidlist_lock);
+
+	do {
+		pid = waitpid(cur->pid, &pstat, 0);
+	} while (pid == -1 && errno == EINTR);
+
 	free(cur);
-		
+
 	return (pid == -1 ? -1 : pstat);
 }

--4605703-10461-1063404607=:2576
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
Content-Disposition: INLINE


-- 
Christian Limpach <chris@pin.lu>

--4605703-10461-1063404607=:2576--