--65163593-18575-1063196370=:3488
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
Content-Disposition: INLINE

On 02 Sep 2003 15:53:14 -0400 "Nathan J. Williams" <nathanw@wasabisystems.com> wrote:

> There are a bunch of problems with SAs and vfork, yes. I'll note that
> I think it's important to make libc's internal users of vfork (popen()
> and system()) work properly, but that there's no general need to make
> vfork() work in a multithreaded process; its semantics simply aren't
> sensible.

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.

If there are no objections, I'd like to commit this after the weekend.

There's still the issue of upcalls happening while the child is still
running on the parent's stack.  An upcall will save the ucontext on the
same stack and thus trash it.  Should we just block/hold all upcalls
between vfork and when execve clears P_PPWAIT?


--65163593-18575-1063196370=:3488
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII; NAME="popen-reentrant.txt"
Content-Disposition: INLINE; FILENAME="popen-reentrant.txt"

Index: lib/libc/gen/popen.c
===================================================================
RCS file: /cvsroot/src/lib/libc/gen/popen.c,v
retrieving revision 1.27
diff -u -r1.27 popen.c
--- lib/libc/gen/popen.c	2003/08/07 16:42:55	1.27
+++ lib/libc/gen/popen.c	2003/09/09 18:13:11
@@ -68,9 +68,19 @@
 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;
+
+extern void __flockfile_internal __P((FILE *, int));
+extern void __funlockfile_internal __P((FILE *, int));
+#endif
+
 FILE *
 popen(command, type)
 	const char *command, *type;
@@ -108,11 +118,23 @@
 		return (NULL);
 	}
 
+	rwlock_wrlock(&pidlist_lock);
+#ifdef _REENTRANT
+	for (old = pidlist; old; old = old->next) {
+		FLOCKFILE(old->fp);
+		old->fd = __sfileno(old->fp);
+	}
+#endif
 	rwlock_rdlock(&__environ_lock);
 	switch (pid = vfork()) {
 	case -1:			/* Error. */
 		serrno = errno;
 		rwlock_unlock(&__environ_lock);
+#ifdef _REENTRANT
+		for (old = pidlist; old; old = old->next)
+			FUNLOCKFILE(old->fp);
+#endif
+		rwlock_unlock(&pidlist_lock);
 		free(cur);
 		(void)close(pdes[0]);
 		(void)close(pdes[1]);
@@ -124,7 +146,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]);
@@ -147,6 +173,10 @@
 		/* NOTREACHED */
 	}
 	rwlock_unlock(&__environ_lock);
+#ifdef _REENTRANT
+	for (old = pidlist; old; old = old->next)
+		FUNLOCKFILE(old->fp);
+#endif
 
 	/* Parent; assume fdopen can't fail. */
 	if (*type == 'r') {
@@ -162,6 +192,7 @@
 	cur->pid =  pid;
 	cur->next = pidlist;
 	pidlist = cur;
+	rwlock_unlock(&pidlist_lock);
 
 	return (iop);
 }
@@ -181,25 +212,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);
 }

--65163593-18575-1063196370=:3488
Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
Content-Disposition: INLINE



-- 
Christian Limpach <chris@pin.lu>

--65163593-18575-1063196370=:3488--