Process create/destroy fixups
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 16 Apr 2014 01:54:41 +0000 (18:54 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 16 Apr 2014 01:56:37 +0000 (18:56 -0700)
If sys_proc_create() failed early, a variety of fragile refcntings and
other bits would be messed up.  Mostly since we want to destroy and we
haven't __proc_readied, but destroy cleans up after both alloc and
ready.

kern/src/process.c
kern/src/syscall.c

index 16e8060..3f75346 100644 (file)
@@ -412,6 +412,7 @@ static int __cb_assert_no_pg(struct proc *p, pte_t *pte, void *va, void *arg)
 static void __proc_free(struct kref *kref)
 {
        struct proc *p = container_of(kref, struct proc, p_kref);
+       void *hash_ret;
        physaddr_t pa;
 
        printd("[PID %d] freeing proc: %d\n", current ? current->pid : 0, p->pid);
@@ -438,10 +439,14 @@ static void __proc_free(struct kref *kref)
        }
        /* Remove us from the pid_hash and give our PID back (in that order). */
        spin_lock(&pid_hash_lock);
-       if (!hashtable_remove(pid_hash, (void*)(long)p->pid))
-               panic("Proc not in the pid table in %s", __FUNCTION__);
+       hash_ret = hashtable_remove(pid_hash, (void*)(long)p->pid);
        spin_unlock(&pid_hash_lock);
-       put_free_pid(p->pid);
+       /* might not be in the hash/ready, if we failed during proc creation */
+       if (hash_ret)
+               put_free_pid(p->pid);
+       else
+               printd("[kernel] pid %d not in the PID hash in %s\n", p->pid,
+                      __FUNCTION__);
        /* all memory below UMAPTOP should have been freed via the VMRs.  the stuff
         * above is the global page and procinfo/procdata */
        env_user_mem_free(p, (void*)UMAPTOP, UVPT - UMAPTOP); /* 3rd arg = len... */
index 0fd1e05..61efccc 100644 (file)
@@ -339,17 +339,25 @@ static int sys_proc_create(struct proc *p, char *path, size_t path_l,
        /* TODO: need to split the proc creation, since you must load after setting
         * args/env, since auxp gets set up there. */
        //new_p = proc_create(program, 0, 0);
-       if (proc_alloc(&new_p, current))
+       if (proc_alloc(&new_p, current)) {
+               set_errstr("Failed to alloc new proc");
                goto mid_error;
+       }
        /* Set the argument stuff needed by glibc */
        if (memcpy_from_user_errno(p, new_p->procinfo->argp, pi->argp,
-                                  sizeof(pi->argp)))
+                                  sizeof(pi->argp))) {
+               set_errstr("Failed to memcpy argp");
                goto late_error;
+       }
        if (memcpy_from_user_errno(p, new_p->procinfo->argbuf, pi->argbuf,
-                                  sizeof(pi->argbuf)))
+                                  sizeof(pi->argbuf))) {
+               set_errstr("Failed to memcpy argbuf");
                goto late_error;
-       if (load_elf(new_p, program))
+       }
+       if (load_elf(new_p, program)) {
+               set_errstr("Failed to load elf");
                goto late_error;
+       }
        kref_put(&program->f_kref);
        /* Connect to stdin, stdout, stderr (part of proc_create()) */
        assert(insert_file(&new_p->open_files, dev_stdin,  0) == 0);
@@ -360,8 +368,12 @@ static int sys_proc_create(struct proc *p, char *path, size_t path_l,
        proc_decref(new_p);     /* give up the reference created in proc_create() */
        return pid;
 late_error:
+       set_errno(EINVAL);
+       /* proc_destroy will decref once, which is for the ref created in
+        * proc_create().  We don't decref again (the usual "+1 for existing"),
+        * since the scheduler, which usually handles that, hasn't heard about the
+        * process (via __proc_ready()). */
        proc_destroy(new_p);
-       proc_decref(new_p);     /* give up the reference created in proc_create() */
 mid_error:
        kref_put(&program->f_kref);
        return -1;