NetBSD Problem Report #53904

From www@NetBSD.org  Thu Jan 24 14:59:52 2019
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 0C7F67A16A
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 24 Jan 2019 14:59:52 +0000 (UTC)
Message-Id: <20190124145951.125507A1ED@mollari.NetBSD.org>
Date: Thu, 24 Jan 2019 14:59:51 +0000 (UTC)
From: zhujtcsieee@gmail.com
Reply-To: zhujtcsieee@gmail.com
To: gnats-bugs@NetBSD.org
Subject: popen function is not thread safe
X-Send-Pr-Version: www-1.0

>Number:         53904
>Category:       lib
>Synopsis:       popen function is not thread safe
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    lib-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 24 15:00:00 +0000 2019
>Closed-Date:    Wed May 17 10:32:43 +0000 2023
>Last-Modified:  Wed May 17 10:32:43 +0000 2023
>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.


>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/53904 CVS commit: src/lib/libc/gen
Date: Thu, 24 Jan 2019 13:01:38 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Thu Jan 24 18:01:38 UTC 2019

 Modified Files:
 	src/lib/libc/gen: popen.c

 Log Message:
 PR/53904: Jintao Zhu: Use a mutex instead of an rwlock to assure thread safety


 To generate a diff of this commit:
 cvs rdiff -u -r1.35 -r1.36 src/lib/libc/gen/popen.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Martin Husemann <martin@duskware.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
Date: Thu, 24 Jan 2019 19:20:09 +0100

 On Thu, Jan 24, 2019 at 06:05:01PM +0000, Christos Zoulas wrote:
 >  Log Message:
 >  PR/53904: Jintao Zhu: Use a mutex instead of an rwlock to assure thread safety

 Slightly unrelated side remark: this should be rewritten using posix_spawn.
 Would be a nice tiny GSoC project.

 Martin

From: Joerg Sonnenberger <joerg@bec.de>
To: gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, zhujtcsieee@gmail.com
Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
Date: Thu, 24 Jan 2019 21:42:31 +0100

 On Thu, Jan 24, 2019 at 06:25:01PM +0000, Martin Husemann wrote:
 > The following reply was made to PR lib/53904; it has been noted by GNATS.
 > 
 > From: Martin Husemann <martin@duskware.de>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
 > Date: Thu, 24 Jan 2019 19:20:09 +0100
 > 
 >  On Thu, Jan 24, 2019 at 06:05:01PM +0000, Christos Zoulas wrote:
 >  >  Log Message:
 >  >  PR/53904: Jintao Zhu: Use a mutex instead of an rwlock to assure thread safety
 >  
 >  Slightly unrelated side remark: this should be rewritten using posix_spawn.
 >  Would be a nice tiny GSoC project.

 It should be using close-on-exec and just drop the whole need for the
 linked list.

 Joerg

From: christos@zoulas.com (Christos Zoulas)
To: Joerg Sonnenberger <joerg@bec.de>, gnats-bugs@NetBSD.org
Cc: lib-bug-people@netbsd.org, gnats-admin@netbsd.org, 
	netbsd-bugs@netbsd.org, zhujtcsieee@gmail.com
Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
Date: Thu, 24 Jan 2019 16:24:12 -0500

 On Jan 24,  9:42pm, joerg@bec.de (Joerg Sonnenberger) wrote:
 -- Subject: Re: PR/53904 CVS commit: src/lib/libc/gen

 | It should be using close-on-exec and just drop the whole need for the
 | linked list.

 It needs extra info from the fp to do pclose() properly (to find the process
 to wait4). Another way to do this is to wrap the fp with funopen() and keep
 the information there... Should be trivial. That would break fileno(fp),
 although that can be also fixed.

 christos

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
Date: Fri, 25 Jan 2019 17:11:04 +0000

 On Thu, Jan 24, 2019 at 08:45:01PM +0000, Joerg Sonnenberger wrote:
  >  [popen] should be using close-on-exec

 Are you sure that's allowed? I know it's dodgy but I wouldn't be
 super-surprised to find someone using popen in conjunction with
 fork/exec.

 -- 
 David A. Holland
 dholland@netbsd.org

From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, lib-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, zhujtcsieee@gmail.com
Cc: 
Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
Date: Fri, 25 Jan 2019 12:40:51 -0500

 On Jan 25,  5:15pm, dholland-bugs@netbsd.org (David Holland) wrote:
 -- Subject: Re: PR/53904 CVS commit: src/lib/libc/gen

 | The following reply was made to PR lib/53904; it has been noted by GNATS.
 | 
 | From: David Holland <dholland-bugs@netbsd.org>
 | To: gnats-bugs@NetBSD.org
 | Cc: 
 | Subject: Re: PR/53904 CVS commit: src/lib/libc/gen
 | Date: Fri, 25 Jan 2019 17:11:04 +0000
 | 
 |  On Thu, Jan 24, 2019 at 08:45:01PM +0000, Joerg Sonnenberger wrote:
 |   >  [popen] should be using close-on-exec
 |  
 |  Are you sure that's allowed? I know it's dodgy but I wouldn't be
 |  super-surprised to find someone using popen in conjunction with
 |  fork/exec.

 The issues are orthogonal. pclose(3) needs to have access to the child
 process info to wait for it. Close-on-exec has nothing to do with it.
 In fact you can select the close-on-exec behavior by adding "e" to the
 "type" argument as mentioned in the man page.

 christos

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/53904 CVS commit: [netbsd-8] src/lib/libc/gen
Date: Sun, 27 Jan 2019 18:25:52 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sun Jan 27 18:25:52 UTC 2019

 Modified Files:
 	src/lib/libc/gen [netbsd-8]: popen.c

 Log Message:
 Pull up following revision(s) (requested by christos in ticket #1170):

 	lib/libc/gen/popen.c: revision 1.36

 PR/53904: Jintao Zhu: Use a mutex instead of an rwlock to assure thread safety


 To generate a diff of this commit:
 cvs rdiff -u -r1.35 -r1.35.8.1 src/lib/libc/gen/popen.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Wed, 17 May 2023 10:32:43 +0000
State-Changed-Why:
fixed in 2019


>Unformatted:

Home	PR Database Search