lib/53904: popen function is not thread safe

[zhujtcsieee%gmail.com@localhost]

>Number:         53904
>Category:       lib
>Synopsis:       popen function is not thread safe
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 24 15:00:00 +0000 2019
>Originator:     Jintao Zhu
>Release:        v 1.35
>Organization:
GM
>Environment:
v 1.35
>Description:
subject: popen() function is not thread safe

source code file : https://nxr.netbsd.org/xref/src/lib/libc/gen/popen.c

version : /*	$NetBSD: popen.c,v 1.35 2015/02/02 22:07:05 christos Exp $	*/

Bug description:
 In the following code snippet, at line 187, acquiring read-lock, that means, at line 162, multiple thread may possibly write "pidlist" at the same time. This race condition may make some "node" not able to be inserted to that list "pidlist", and thereafter, pclose may fail due to it not able to find the "node" in that list(at line 278); therefore, the FILE* iop not able to be closed(at line 290) and the child process not able to be reaped(at line 303)


//-----------------------------------------------------------------------
     66 static struct pid {
     67 	struct pid *next;
     68 	FILE *fp;
     69 #ifdef _REENTRANT
     70 	int fd;
     71 #endif
     72 	pid_t pid;
     73 } *pidlist;                                         // the list

     76 static rwlock_t pidlist_lock = RWLOCK_INITIALIZER;  // the lock

//-----------------------------------------------------------------------
    173 FILE *
    174 popen(const char *cmd, const char *type)
    175 {
    187 	(void)rwlock_rdlock(&pidlist_lock);             // acquiring read-lock
            ....
    209 	pdes_parent(pdes, cur, pid, type);
    210     ...
    212 	(void)rwlock_unlock(&pidlist_lock);
            ...


//-----------------------------------------------------------------------
    138 static void
    139 pdes_parent(int *pdes, struct pid *cur, pid_t pid, const char *type)
    140 {
            ...
    158 	/* Link into list of file descriptors. */
    159 	cur->fp = iop;
    160 	cur->pid =  pid;
    161 	cur->next = pidlist;
    162 	pidlist = cur;                                  // write to pidlist
    163 }

//-----------------------------------------------------------------------
    265 int
    266 pclose(FILE *iop)
    267 {
            ...
    278 	/* Find the appropriate file pointer. */                    // searching file pointer
    279 	for (last = NULL, cur = pidlist; cur; last = cur, cur = cur->next)
    280 		if (cur->fp == iop)
    281 			break;
    282 	if (cur == NULL) {
                ...
    286 		errno = ESRCH;
    287 		return -1;
    288 	}
    289 
    290 	(void)fclose(iop);
                ...
    303 		pid = waitpid(cur->pid, &pstat, 0);
                ...
    309 }

>How-To-Repeat:
unit test code
>Fix:
Use a normal mutex(lock) instead of a read-write lock.