thread-safe popen(3) at 4-STABLE

[Dmitry Morozovsky]

Dear colleagues,

checking for hanged and CPU-eating clamav, my colleague (oleg at rinet.ru) found 
that popen(3) is not thread safe. What about commiting tjr's fix (attached) to 
RELENG_4?

Sincerely,
D.Marck                                        [DM5020, MCK-RIPE, DM3-RIPN]
---------------------------------------------------------------------------
*** Dmitry Morozovsky --- D.Marck --- Wild Woozle --- marck at FreeBSD.org ***
---------------------------------------------------------------------------
-------------- next part --------------
Index: lib/libc/gen/popen.c
===================================================================
RCS file: /home/ncvs/src/lib/libc/gen/popen.c,v
retrieving revision 1.14
diff -u -r1.14 popen.c
--- lib/libc/gen/popen.c	27 Jan 2000 23:06:19 -0000	1.14
+++ lib/libc/gen/popen.c	16 Oct 2004 16:14:38 -0000
@@ -51,6 +51,17 @@
 #include <string.h>
 #include <paths.h>
 
+#ifndef _THREAD_SAFE
+#define THREAD_LOCK()
+#define THREAD_UNLOCK()
+#else
+#include <pthread.h>
+#include "libc_private.h"
+static pthread_mutex_t pidlist_mutex = PTHREAD_MUTEX_INITIALIZER;
+#define	THREAD_LOCK()	if (__isthreaded) _pthread_mutex_lock(&pidlist_mutex)
+#define	THREAD_UNLOCK()	if (__isthreaded) _pthread_mutex_unlock(&pidlist_mutex)
+#endif /* _THREAD_SAFE */
+
 extern char **environ;
 
 static struct pid {
@@ -95,8 +106,10 @@
 	argv[2] = (char *)command;
 	argv[3] = NULL;
 
+	THREAD_LOCK();
 	switch (pid = vfork()) {
 	case -1:			/* Error. */
+		THREAD_UNLOCK();
 		(void)_close(pdes[0]);
 		(void)_close(pdes[1]);
 		free(cur);
@@ -134,6 +147,7 @@
 		_exit(127);
 		/* NOTREACHED */
 	}
+	THREAD_UNLOCK();
 
 	/* Parent; assume fdopen can't fail. */
 	if (*type == 'r') {
@@ -146,9 +160,11 @@
 
 	/* Link into list of file descriptors. */
 	cur->fp = iop;
-	cur->pid =  pid;
+	cur->pid = pid;
+	THREAD_LOCK();
 	cur->next = pidlist;
 	pidlist = cur;
+	THREAD_UNLOCK();
 
 	return (iop);
 }
@@ -167,12 +183,23 @@
 	int pstat;
 	pid_t pid;
 
-	/* Find the appropriate file pointer. */
+	/*
+	 * Find the appropriate file pointer and remove it from the list.
+	 */
+	THREAD_LOCK();
 	for (last = NULL, cur = pidlist; cur; last = cur, cur = cur->next)
 		if (cur->fp == iop)
 			break;
-	if (cur == NULL)
+	if (cur == NULL) {
+		THREAD_UNLOCK();
 		return (-1);
+	}
+	/* Remove the entry from the linked list. */
+	if (last == NULL)
+		pidlist = cur->next;
+	else
+		last->next = cur->next;
+	THREAD_UNLOCK();
 
 	(void)fclose(iop);
 
@@ -180,11 +207,6 @@
 		pid = _wait4(cur->pid, &pstat, 0, (struct rusage *)0);
 	} while (pid == -1 && errno == EINTR);
 
-	/* Remove the entry from the linked list. */
-	if (last == NULL)
-		pidlist = cur->next;
-	else
-		last->next = cur->next;
 	free(cur);
 
 	return (pid == -1 ? -1 : pstat);