Protect pidlist with a mutex to avoid a race causing a duplicate free()

when the same pipe FILE is pclosed()'d in different threads, and to avoid
corrupting the linked list when adding or removing items. The symptoms of
the linked list getting corrupted were pclose() either not finding the pipe
on the list, or the list becoming circular and pclose() looping infinitely.

1 files changed, 24 insertions, 8 deletions

diff --git a/lib/libc/gen/popen.c b/lib/libc/gen/popen.c
index 82a01bb..45d1bcf 100644
--- a/lib/libc/gen/popen.c
+++ b/lib/libc/gen/popen.c
@@ -51,7 +51,9 @@ __FBSDID("$FreeBSD$");
 #include <stdlib.h>
 #include <string.h>
 #include <paths.h>
+#include <pthread.h>
 #include "un-namespace.h"
+#include "libc_private.h"
 
 extern char **environ;
 
@@ -60,6 +62,10 @@ static struct pid {
 	FILE *fp;
 	pid_t pid;
 } *pidlist;
+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)
 
 FILE *
 popen(command, type)
@@ -97,8 +103,10 @@ popen(command, type)
 	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);
@@ -136,6 +144,7 @@ popen(command, type)
 		_exit(127);
 		/* NOTREACHED */
 	}
+	THREAD_UNLOCK();
 
 	/* Parent; assume fdopen can't fail. */
 	if (*type == 'r') {
@@ -148,9 +157,11 @@ popen(command, type)
 
 	/* 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);
 }
@@ -169,12 +180,22 @@ pclose(iop)
 	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);
+	}
+	if (last == NULL)
+		pidlist = cur->next;
+	else
+		last->next = cur->next;
+	THREAD_UNLOCK();
 
 	(void)fclose(iop);
 
@@ -182,11 +203,6 @@ pclose(iop)
 		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);