Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/netbsd-6]: src Pull up the following revisions(s) (requested by martin i...



details:   https://anonhg.NetBSD.org/src/rev/f77b66baaaa5
branches:  netbsd-6
changeset: 773782:f77b66baaaa5
user:      sborrill <sborrill%NetBSD.org@localhost>
date:      Mon Feb 20 21:54:55 2012 +0000

description:
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.

diffstat:

 include/spawn.h                                |    3 +-
 sys/kern/kern_exec.c                           |  128 ++++++++++++++++--------
 sys/uvm/uvm_glue.c                             |    9 +-
 tests/lib/libc/gen/posix_spawn/t_fileactions.c |   47 ++++++++-
 4 files changed, 134 insertions(+), 53 deletions(-)

diffs (truncated from 336 to 300 lines):

diff -r c88612620f5d -r f77b66baaaa5 include/spawn.h
--- a/include/spawn.h   Mon Feb 20 21:42:13 2012 +0000
+++ b/include/spawn.h   Mon Feb 20 21:54:55 2012 +0000
@@ -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 <ed%FreeBSD.org@localhost>
@@ -33,6 +33,7 @@
 
 #include <sys/spawn.h>
 
+__BEGIN_DECLS
 /*
  * Spawn routines
  *
diff -r c88612620f5d -r f77b66baaaa5 sys/kern/kern_exec.c
--- a/sys/kern/kern_exec.c      Mon Feb 20 21:42:13 2012 +0000
+++ b/sys/kern/kern_exec.c      Mon Feb 20 21:54:55 2012 +0000
@@ -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 @@
        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 @@
        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 @@
                }
        }
 
-       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 @@
        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 @@
                        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 @@
         * 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 @@
 #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 @@
        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 @@
                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));
        }
 
diff -r c88612620f5d -r f77b66baaaa5 sys/uvm/uvm_glue.c
--- a/sys/uvm/uvm_glue.c        Mon Feb 20 21:42:13 2012 +0000
+++ b/sys/uvm/uvm_glue.c        Mon Feb 20 21:54:55 2012 +0000
@@ -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 @@
 
        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);
diff -r c88612620f5d -r f77b66baaaa5 tests/lib/libc/gen/posix_spawn/t_fileactions.c
--- a/tests/lib/libc/gen/posix_spawn/t_fileactions.c    Mon Feb 20 21:42:13 2012 +0000
+++ b/tests/lib/libc/gen/posix_spawn/t_fileactions.c    Mon Feb 20 21:54:55 2012 +0000
@@ -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_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;
+
+       /*



Home | Main Index | Thread Index | Old Index