Module Name: src
Committed By: sborrill
Date: Mon Feb 20 21:54:57 UTC 2012
Modified Files:
src/include [netbsd-6]: spawn.h
src/sys/kern [netbsd-6]: kern_exec.c
src/sys/uvm [netbsd-6]: uvm_glue.c
src/tests/lib/libc/gen/posix_spawn [netbsd-6]: t_fileactions.c
Log Message:
Pull up the following revisions(s) (requested by martin in ticket #14):
include/spawn.h: revision 1.2
sys/kern/kern_exec.c: revision 1.341
sys/uvm/uvm_glue.c: revision 1.157
tests/lib/libc/gen/posix_spawn/t_fileactions.c: revision 1.3
posix_spawn: fix kernel bug when passing empty fileactions (PR kern/46038)
and add a test case for this. Fix potential race condition, doublefreeing
of memory and memory leaks in error cases.
To generate a diff of this commit:
cvs rdiff -u -r1.1 -r1.1.2.1 src/include/spawn.h
cvs rdiff -u -r1.339 -r1.339.2.1 src/sys/kern/kern_exec.c
cvs rdiff -u -r1.156 -r1.156.2.1 src/sys/uvm/uvm_glue.c
cvs rdiff -u -r1.2 -r1.2.2.1 \
src/tests/lib/libc/gen/posix_spawn/t_fileactions.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/include/spawn.h
diff -u src/include/spawn.h:1.1 src/include/spawn.h:1.1.2.1
--- src/include/spawn.h:1.1 Sat Feb 11 23:31:24 2012
+++ src/include/spawn.h Mon Feb 20 21:54:55 2012
@@ -1,4 +1,4 @@
-/* $NetBSD: spawn.h,v 1.1 2012/02/11 23:31:24 martin Exp $ */
+/* $NetBSD: spawn.h,v 1.1.2.1 2012/02/20 21:54:55 sborrill Exp $ */
/*-
* Copyright (c) 2008 Ed Schouten <e...@freebsd.org>
@@ -33,6 +33,7 @@
#include <sys/spawn.h>
+__BEGIN_DECLS
/*
* Spawn routines
*
Index: src/sys/kern/kern_exec.c
diff -u src/sys/kern/kern_exec.c:1.339 src/sys/kern/kern_exec.c:1.339.2.1
--- src/sys/kern/kern_exec.c:1.339 Sun Feb 12 20:11:03 2012
+++ src/sys/kern/kern_exec.c Mon Feb 20 21:54:56 2012
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_exec.c,v 1.339 2012/02/12 20:11:03 martin Exp $ */
+/* $NetBSD: kern_exec.c,v 1.339.2.1 2012/02/20 21:54:56 sborrill Exp $ */
/*-
* Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -59,7 +59,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.339 2012/02/12 20:11:03 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.339.2.1 2012/02/20 21:54:56 sborrill Exp $");
#include "opt_exec.h"
#include "opt_ktrace.h"
@@ -1351,8 +1351,7 @@ execve_runproc(struct lwp *l, struct exe
ktremul();
/* Allow new references from the debugger/procfs. */
- if (!proc_is_new)
- rw_exit(&p->p_reflock);
+ rw_exit(&p->p_reflock);
rw_exit(&exec_lock);
mutex_enter(proc_lock);
@@ -1777,6 +1776,20 @@ spawn_return(void *arg)
size_t i;
const struct posix_spawn_file_actions_entry *fae;
register_t retval;
+ bool have_reflock;
+
+ /*
+ * The following actions may block, so we need a temporary
+ * vmspace - borrow the kernel one
+ */
+ KPREEMPT_DISABLE(l);
+ l->l_proc->p_vmspace = proc0.p_vmspace;
+ pmap_activate(l);
+ KPREEMPT_ENABLE(l);
+
+ /* don't allow debugger access yet */
+ rw_enter(&l->l_proc->p_reflock, RW_WRITER);
+ have_reflock = true;
error = 0;
/* handle posix_spawn_file_actions */
@@ -1891,18 +1904,17 @@ spawn_return(void *arg)
}
}
- if (spawn_data->sed_actions != NULL) {
- for (i = 0; i < spawn_data->sed_actions_len; i++) {
- fae = &spawn_data->sed_actions[i];
- if (fae->fae_action == FAE_OPEN)
- kmem_free(fae->fae_path,
- strlen(fae->fae_path)+1);
- }
- }
+ /* stop using kernel vmspace */
+ KPREEMPT_DISABLE(l);
+ pmap_deactivate(l);
+ l->l_proc->p_vmspace = NULL;
+ KPREEMPT_ENABLE(l);
+
/* now do the real exec */
rw_enter(&exec_lock, RW_READER);
error = execve_runproc(l, &spawn_data->sed_exec);
+ have_reflock = false;
if (error == EJUSTRETURN)
error = 0;
else if (error)
@@ -1920,13 +1932,15 @@ spawn_return(void *arg)
return;
report_error:
- if (spawn_data->sed_actions != NULL) {
- for (i = 0; i < spawn_data->sed_actions_len; i++) {
- fae = &spawn_data->sed_actions[i];
- if (fae->fae_action == FAE_OPEN)
- kmem_free(fae->fae_path,
- strlen(fae->fae_path)+1);
- }
+ if (have_reflock)
+ rw_exit(&l->l_proc->p_reflock);
+
+ /* stop using kernel vmspace (if we haven't already) */
+ if (l->l_proc->p_vmspace) {
+ KPREEMPT_DISABLE(l);
+ pmap_deactivate(l);
+ l->l_proc->p_vmspace = NULL;
+ KPREEMPT_ENABLE(l);
}
/*
@@ -2007,27 +2021,31 @@ sys_posix_spawn(struct lwp *l1, const st
fa->fae = NULL;
goto error_exit;
}
- ufa = fa->fae;
- fa->fae = kmem_alloc(fa->len *
- sizeof(struct posix_spawn_file_actions_entry), KM_SLEEP);
- error = copyin(ufa, fa->fae,
- fa->len * sizeof(struct posix_spawn_file_actions_entry));
- if (error)
- goto error_exit;
- for (i = 0; i < fa->len; i++) {
- if (fa->fae[i].fae_action == FAE_OPEN) {
- char buf[PATH_MAX];
- error = copyinstr(fa->fae[i].fae_path, buf,
- sizeof(buf), NULL);
- if (error)
- break;
- fa->fae[i].fae_path = kmem_alloc(strlen(buf)+1,
- KM_SLEEP);
- if (fa->fae[i].fae_path == NULL) {
- error = ENOMEM;
- break;
+ if (fa->len) {
+ ufa = fa->fae;
+ fa->fae = kmem_alloc(fa->len *
+ sizeof(struct posix_spawn_file_actions_entry),
+ KM_SLEEP);
+ error = copyin(ufa, fa->fae,
+ fa->len *
+ sizeof(struct posix_spawn_file_actions_entry));
+ if (error)
+ goto error_exit;
+ for (i = 0; i < fa->len; i++) {
+ if (fa->fae[i].fae_action == FAE_OPEN) {
+ char buf[PATH_MAX];
+ error = copyinstr(fa->fae[i].fae_path,
+ buf, sizeof(buf), NULL);
+ if (error)
+ break;
+ fa->fae[i].fae_path = kmem_alloc(
+ strlen(buf)+1, KM_SLEEP);
+ if (fa->fae[i].fae_path == NULL) {
+ error = ENOMEM;
+ break;
+ }
+ strcpy(fa->fae[i].fae_path, buf);
}
- strcpy(fa->fae[i].fae_path, buf);
}
}
}
@@ -2183,8 +2201,10 @@ sys_posix_spawn(struct lwp *l1, const st
* Prepare remaining parts of spawn data
*/
if (fa != NULL) {
- spawn_data->sed_actions_len = fa->len;
- spawn_data->sed_actions = fa->fae;
+ if (fa->len) {
+ spawn_data->sed_actions_len = fa->len;
+ spawn_data->sed_actions = fa->fae;
+ }
kmem_free(fa, sizeof(*fa));
fa = NULL;
}
@@ -2246,7 +2266,6 @@ sys_posix_spawn(struct lwp *l1, const st
#ifdef __HAVE_SYSCALL_INTERN
(*p2->p_emul->e_syscall_intern)(p2);
#endif
- rw_exit(&p1->p_reflock);
/*
* Make child runnable, set start time, and add to run queue except
@@ -2271,15 +2290,25 @@ sys_posix_spawn(struct lwp *l1, const st
mutex_exit(&spawn_data->sed_mtx_child);
error = spawn_data->sed_error;
+ rw_exit(&p1->p_reflock);
rw_exit(&exec_lock);
have_exec_lock = false;
- if (spawn_data->sed_actions != NULL)
+ if (spawn_data->sed_actions != NULL) {
+ for (i = 0; i < spawn_data->sed_actions_len; i++) {
+ if (spawn_data->sed_actions[i].fae_action == FAE_OPEN)
+ kmem_free(spawn_data->sed_actions[i].fae_path,
+ strlen(spawn_data->sed_actions[i].fae_path)
+ +1);
+ }
kmem_free(spawn_data->sed_actions,
- spawn_data->sed_actions_len * sizeof(*spawn_data->sed_actions));
+ spawn_data->sed_actions_len
+ * sizeof(*spawn_data->sed_actions));
+ }
if (spawn_data->sed_attrs != NULL)
- kmem_free(spawn_data->sed_attrs, sizeof(*spawn_data->sed_attrs));
+ kmem_free(spawn_data->sed_attrs,
+ sizeof(*spawn_data->sed_attrs));
cv_destroy(&spawn_data->sed_cv_child_ready);
mutex_destroy(&spawn_data->sed_mtx_child);
@@ -2297,8 +2326,15 @@ sys_posix_spawn(struct lwp *l1, const st
rw_exit(&exec_lock);
if (fa != NULL) {
- if (fa->fae != NULL)
+ if (fa->fae != NULL) {
+ for (i = 0; i < fa->len; i++) {
+ if (fa->fae->fae_action == FAE_OPEN
+ && fa->fae->fae_path != NULL)
+ kmem_free(fa->fae->fae_path,
+ strlen(fa->fae->fae_path)+1);
+ }
kmem_free(fa->fae, fa->len * sizeof(*fa->fae));
+ }
kmem_free(fa, sizeof(*fa));
}
Index: src/sys/uvm/uvm_glue.c
diff -u src/sys/uvm/uvm_glue.c:1.156 src/sys/uvm/uvm_glue.c:1.156.2.1
--- src/sys/uvm/uvm_glue.c:1.156 Sun Feb 12 11:18:04 2012
+++ src/sys/uvm/uvm_glue.c Mon Feb 20 21:54:57 2012
@@ -1,4 +1,4 @@
-/* $NetBSD: uvm_glue.c,v 1.156 2012/02/12 11:18:04 martin Exp $ */
+/* $NetBSD: uvm_glue.c,v 1.156.2.1 2012/02/20 21:54:57 sborrill Exp $ */
/*
* Copyright (c) 1997 Charles D. Cranor and Washington University.
@@ -62,7 +62,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: uvm_glue.c,v 1.156 2012/02/12 11:18:04 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: uvm_glue.c,v 1.156.2.1 2012/02/20 21:54:57 sborrill Exp $");
#include "opt_kgdb.h"
#include "opt_kstack.h"
@@ -416,14 +416,13 @@ uvm_proc_exit(struct proc *p)
KASSERT(p == l->l_proc);
ovm = p->p_vmspace;
- if (__predict_false(ovm == NULL))
- return;
/*
* borrow proc0's address space.
*/
KPREEMPT_DISABLE(l);
- pmap_deactivate(l);
+ if (__predict_true(ovm != NULL))
+ pmap_deactivate(l);
p->p_vmspace = proc0.p_vmspace;
pmap_activate(l);
KPREEMPT_ENABLE(l);
Index: src/tests/lib/libc/gen/posix_spawn/t_fileactions.c
diff -u src/tests/lib/libc/gen/posix_spawn/t_fileactions.c:1.2 src/tests/lib/libc/gen/posix_spawn/t_fileactions.c:1.2.2.1
--- src/tests/lib/libc/gen/posix_spawn/t_fileactions.c:1.2 Tue Feb 14 00:13:54 2012
+++ src/tests/lib/libc/gen/posix_spawn/t_fileactions.c Mon Feb 20 21:54:57 2012
@@ -1,4 +1,4 @@
-/* $NetBSD: t_fileactions.c,v 1.2 2012/02/14 00:13:54 martin Exp $ */
+/* $NetBSD: t_fileactions.c,v 1.2.2.1 2012/02/20 21:54:57 sborrill Exp $ */
/*-
* Copyright (c) 2012 The NetBSD Foundation, Inc.
@@ -279,12 +279,57 @@ ATF_TC_BODY(t_spawn_fileactions, tc)
ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == EXIT_SUCCESS);
}
+ATF_TC(t_spawn_empty_fileactions);
+
+ATF_TC_HEAD(t_spawn_empty_fileactions, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "posix_spawn with empty fileactions (PR kern/46038)");
+ atf_tc_set_md_var(tc, "require.progs", "/bin/cat");
+}
+
+ATF_TC_BODY(t_spawn_empty_fileactions, tc)
+{
+ int status, err;
+ pid_t pid;
+ char * const args[2] = { __UNCONST("cat"), NULL };
+ posix_spawn_file_actions_t fa;
+ size_t insize, outsize;
+
+ /*
+ * try a "cat < testfile > checkfile", but set up stdin/stdout
+ * already in the parent and pass empty file actions to the child.
+ */
+ make_testfile(TESTFILE);
+ unlink(CHECKFILE);
+
+ freopen(TESTFILE, "r", stdin);
+ freopen(CHECKFILE, "w", stdout);
+
+ posix_spawn_file_actions_init(&fa);
+ err = posix_spawn(&pid, "/bin/cat", &fa, NULL, args, NULL);
+ posix_spawn_file_actions_destroy(&fa);
+
+ ATF_REQUIRE(err == 0);
+
+ /* ok, wait for the child to finish */
+ waitpid(pid, &status, 0);
+ ATF_REQUIRE(WIFEXITED(status) && WEXITSTATUS(status) == EXIT_SUCCESS);
+
+ /* now check that input and output have the same size */
+ insize = filesize(TESTFILE);
+ outsize = filesize(CHECKFILE);
+ ATF_REQUIRE(insize == strlen(TESTCONTENT));
+ ATF_REQUIRE(insize == outsize);
+}
+
ATF_TP_ADD_TCS(tp)
{
ATF_TP_ADD_TC(tp, t_spawn_fileactions);
ATF_TP_ADD_TC(tp, t_spawn_open_nonexistent);
ATF_TP_ADD_TC(tp, t_spawn_reopen);
ATF_TP_ADD_TC(tp, t_spawn_openmode);
+ ATF_TP_ADD_TC(tp, t_spawn_empty_fileactions);
return atf_no_error();
}
