NetBSD Problem Report #50330

From paul@pokey.whooppee.com  Sun Oct 11 23:28:19 2015
Return-Path: <paul@pokey.whooppee.com>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 3F196A5864
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 11 Oct 2015 23:28:19 +0000 (UTC)
Message-Id: <20151011232733.39AD916E7C@pokey.whooppee.com>
Date: Mon, 12 Oct 2015 07:27:33 +0800 (PHT)
From: paul@whooppee.com
Reply-To: paul@whooppee.com
To: gnats-bugs@NetBSD.org
Subject: spawn_return alters p_stat without updating p_nstopchild
X-Send-Pr-Version: 3.95

>Number:         50330
>Category:       kern
>Synopsis:       spawn_return alters p_stat without updating p_nstopchild
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    pgoyette
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Oct 11 23:30:00 +0000 2015
>Closed-Date:    Sat Nov 07 22:20:17 +0000 2015
>Last-Modified:  Sat Nov 07 22:20:17 +0000 2015
>Originator:     paul@whooppee.com
>Release:        NetBSD 7.99.21
>Organization:
+------------------+--------------------------+-------------------------+
| Paul Goyette     | PGP Key fingerprint:     | E-mail addresses:       |
| (Retired)        | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com    |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org  |
+------------------+--------------------------+-------------------------+
>Environment:


System: NetBSD pokey.whooppee.com 7.99.21 NetBSD 7.99.21 (POKEY 2015-10-11 11:09:10) #2: Sun Oct 11 20:45:56 PHT 2015 paul@pokey.whooppee.com:/build/netbsd-local/obj/amd64/sys/arch/amd64/compile/POKEY amd64
Architecture: x86_64
Machine: amd64
>Description:
	In src/sys/kern/kern_exec.c routine spawn_return(), the newly-
	spawned process's p_stat is temporarily changed from SACTIVE to
	SSTOP, without corresponding adjustments to either p_waited or
	its parent's p_nstopchild.  In the normal (non-error) path,
	fairly soon after, p_stat is restored to its original value, again
	without adjusting the other values.  There is thus a small window
	where these values may be inconsistent.

	If, during this window, other errors with the spawn are found,
	we call exit1(), where we will eventually set the process's
	p_stat to SDEAD and make necessary adjustments to p_waited and
	(parent's) p_nstopchild.  The window is thus closed, but still
	exists, when the values are inconsistent.

>How-To-Repeat:
	Found by code inspection.

>Fix:

Index: kern_exec.c
===================================================================
RCS file: /cvsroot/src/sys/kern/kern_exec.c,v
retrieving revision 1.418
diff -u -p -r1.418 kern_exec.c
--- kern_exec.c	2 Oct 2015 16:54:15 -0000	1.418
+++ kern_exec.c	11 Oct 2015 12:13:01 -0000
@@ -1983,6 +1983,7 @@ spawn_return(void *arg)
 	struct spawn_exec_data *spawn_data = arg;
 	struct lwp *l = curlwp;
 	int error, newfd;
+	int ostat;
 	size_t i;
 	const struct posix_spawn_file_actions_entry *fae;
 	pid_t ppid;
@@ -2055,7 +2056,6 @@ spawn_return(void *arg)

 	/* handle posix_spawnattr */
 	if (spawn_data->sed_attrs != NULL) {
-		int ostat;
 		struct sigaction sigact;
 		sigact._sa_u._sa_handler = SIG_DFL;
 		sigact.sa_flags = 0;
@@ -2064,8 +2064,18 @@ spawn_return(void *arg)
 		 * set state to SSTOP so that this proc can be found by pid.
 		 * see proc_enterprp, do_sched_setparam below
 		 */
+		mutex_enter(proc_lock);
+		/*
+		 * p_stat should be SACTIVE, so we need to adjust the
+		 * parent's p_nstopchild here.  For safety, just make
+		 * we're on the good side of dead before we adjust.
+		 */
 		ostat = l->l_proc->p_stat;
+		KASSERT(ostat < SSTOP);
 		l->l_proc->p_stat = SSTOP;
+		l->l_proc->p_waited = 0;
+		l->l_proc->p_pptr->p_nstopchild++;
+		mutex_exit(proc_lock);

 		/* Set process group */
 		if (spawn_data->sed_attrs->sa_flags & POSIX_SPAWN_SETPGROUP) {
@@ -2078,7 +2088,7 @@ spawn_return(void *arg)
 			error = proc_enterpgrp(spawn_data->sed_parent,
 			    mypid, pgrp, false);
 			if (error)
-				goto report_error;
+				goto report_error_stopped;
 		}

 		/* Set scheduler policy */
@@ -2092,7 +2102,7 @@ spawn_return(void *arg)
 			    SCHED_NONE, &spawn_data->sed_attrs->sa_schedparam);
 		}
 		if (error)
-			goto report_error;
+			goto report_error_stopped;

 		/* Reset user ID's */
 		if (spawn_data->sed_attrs->sa_flags & POSIX_SPAWN_RESETIDS) {
@@ -2100,12 +2110,12 @@ spawn_return(void *arg)
 			     kauth_cred_getgid(l->l_cred), -1,
 			     ID_E_EQ_R | ID_E_EQ_S);
 			if (error)
-				goto report_error;
+				goto report_error_stopped;
 			error = do_setresuid(l, -1,
 			    kauth_cred_getuid(l->l_cred), -1,
 			    ID_E_EQ_R | ID_E_EQ_S);
 			if (error)
-				goto report_error;
+				goto report_error_stopped;
 		}

 		/* Set signal masks/defaults */
@@ -2115,7 +2125,7 @@ spawn_return(void *arg)
 			    &spawn_data->sed_attrs->sa_sigmask, NULL);
 			mutex_exit(l->l_proc->p_lock);
 			if (error)
-				goto report_error;
+				goto report_error_stopped;
 		}

 		if (spawn_data->sed_attrs->sa_flags & POSIX_SPAWN_SETSIGDEF) {
@@ -2136,7 +2146,10 @@ spawn_return(void *arg)
 					    0);
 			}
 		}
+		mutex_enter(proc_lock);
 		l->l_proc->p_stat = ostat;
+		l->l_proc->p_pptr->p_nstopchild--;
+		mutex_exit(proc_lock);
 	}

 	/* now do the real exec */
@@ -2163,6 +2176,11 @@ spawn_return(void *arg)
 	/* NOTREACHED */
 	return;

+report_error_stopped:
+	mutex_enter(proc_lock);
+	l->l_proc->p_stat = ostat;
+	l->l_proc->p_pptr->p_nstopchild--;
+	mutex_exit(proc_lock);
  report_error:
 	if (have_reflock) {
 		/*

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->pgoyette
Responsible-Changed-By: pgoyette@NetBSD.org
Responsible-Changed-When: Sun, 11 Oct 2015 23:32:32 +0000
Responsible-Changed-Why:
I have the fix.


From: "Paul Goyette" <pgoyette@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/50330 CVS commit: src/sys/kern
Date: Tue, 13 Oct 2015 00:29:35 +0000

 Module Name:	src
 Committed By:	pgoyette
 Date:		Tue Oct 13 00:29:35 UTC 2015

 Modified Files:
 	src/sys/kern: kern_exec.c

 Log Message:
 In spawn_return() we temporarily move the process state to SSTOP, but
 without updating its p_waited value or its parent's p_nstopchild
 counter.  Later, we restore the original state, again without any
 adjustment of the related values.  This leaves a relatively short
 window when the values are inconsistent and could interfere with the
 proper operation of sys_wait() for the parent (if it manages to be
 scheduled;  it's not totally clear what, if anything, prevents
 scheduling/execution of the parent).

 If during this window, any of the checks being made result in an
 error, we call exit1() which will eventually migrate the process's
 state to SDEAD (with an intermediate transition to SDYING).  At
 this point the other variables get updated, and we finally restore
 a consistent state.

 This change updates the p_waited and parent's p_nstopchild at each
 step to eliminate any windows during which the values could lead to
 incorrect decisions.

 Fixes PR kern/50330

 Pullups will be requested for NetBSD-7, -6, -6-0, and -6-1


 To generate a diff of this commit:
 cvs rdiff -u -r1.419 -r1.420 src/sys/kern/kern_exec.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->pending-pullups
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Tue, 13 Oct 2015 01:35:58 +0000
State-Changed-Why:
Committed to head, pending pullups


State-Changed-From-To: pending-pullups->closed
State-Changed-By: pgoyette@NetBSD.org
State-Changed-When: Sat, 07 Nov 2015 22:20:17 +0000
State-Changed-Why:
Fixed, pullups completed.


>Unformatted:

Home	PR Database Search