Fixes double-free when destroying a process
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 4 Feb 2010 03:07:33 +0000 (19:07 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 4 Feb 2010 03:07:33 +0000 (19:07 -0800)
Just free up to UVPT.  memwalking through UVPT (or VPT) will lead to
headaches and usually something other than what you want.

kern/arch/i386/env.c
kern/src/env.c
kern/src/page_alloc.c
kern/src/process.c

index cdd11f9..33f8d1f 100644 (file)
@@ -83,10 +83,11 @@ void env_pop_tf(trapframe_t *tf)
        }
 }
 
-// Flush all mapped pages in the user portion of the address space
-int
-env_user_mem_walk(env_t* e, void* start, size_t len,
-                  mem_walk_callback_t callback, void* arg)
+/* Walks len bytes from start, executing 'callback' on every PTE, passing it a
+ * specific VA and whatever arg is passed in.  Note, this cannot handle jumbo
+ * pages. */
+int env_user_mem_walk(env_t* e, void* start, size_t len,
+                      mem_walk_callback_t callback, void* arg)
 {
        pte_t *pt;
        uint32_t pdeno, pteno;
@@ -96,21 +97,20 @@ env_user_mem_walk(env_t* e, void* start, size_t len,
        void* end = (char*)start+len;
        uint32_t pdeno_start = PDX(start);
        uint32_t pdeno_end = PDX(ROUNDUP(end,PTSIZE));
+       /* concerned about overflow.  this should catch it for now, given the above
+        * assert. */
+       assert((len == 0) || (pdeno_start < pdeno_end));
 
        for (pdeno = pdeno_start; pdeno < pdeno_end; pdeno++) {
-
-               // only look at mapped page tables
                if (!(e->env_pgdir[pdeno] & PTE_P))
                        continue;
-
-               // find the pa and va of the page table
+               /* find the pa and a pointer to the page table */
                pa = PTE_ADDR(e->env_pgdir[pdeno]);
                pt = (pte_t*COUNT(NPTENTRIES)) KADDR(pa);
-
-               // unmap all PTEs in this page table 
-               uint32_t pteno_start = pdeno == pdeno_start ? PTX(start) : 0;
-               uint32_t pteno_end = pdeno == pdeno_end-1 && PTX(end) != 0 ?
-                                    PTX(end) : NPTENTRIES;
+               /* figure out where we start and end within the page table */
+               uint32_t pteno_start = (pdeno == pdeno_start ? PTX(start) : 0);
+               uint32_t pteno_end = (pdeno == pdeno_end - 1 && PTX(end) != 0 ?
+                                     PTX(end) : NPTENTRIES );
                int ret;
                for (pteno = pteno_start; pteno < pteno_end; pteno++) {
                        if (pt[pteno] & PTE_P)
@@ -121,10 +121,12 @@ env_user_mem_walk(env_t* e, void* start, size_t len,
        return 0;
 }
 
-void
-env_pagetable_free(env_t* e)
+/* Frees (decrefs) all pages of the process's page table, including the page
+ * directory.  Does not free the memory that is actually mapped. */
+void env_pagetable_free(env_t* e)
 {
        static_assert(UVPT % PTSIZE == 0);
+       assert(e->env_cr3 != rcr3());
        for(uint32_t pdeno = 0; pdeno < PDX(UVPT); pdeno++)
        {
                // only look at mapped page tables
index b01eb24..9260c43 100644 (file)
@@ -121,8 +121,8 @@ env_setup_vm_error:
 env_setup_vm_error_d:
        free_cont_pages(e->env_procinfo, LOG2_UP(PROCINFO_NUM_PAGES));
 env_setup_vm_error_i:
-       page_free(shared_page);
-       env_user_mem_free(e,0,KERNBASE);
+       page_decref(shared_page);
+       env_user_mem_free(e, 0, UVPT);
        env_pagetable_free(e);
        return -ENOMEM;
 }
@@ -342,9 +342,10 @@ void run_env_handler(trapframe_t *tf, void * data)
                panic("Failed to enqueue work!");
 }
 
-void
-env_user_mem_free(env_t* e, void* start, size_t len)
+/* Frees (decrefs) all memory mapped in the given range */
+void env_user_mem_free(env_t* e, void* start, size_t len)
 {
+       assert((uintptr_t)start + len <= UVPT); //since this keeps fucking happening
        int user_page_free(env_t* e, pte_t* pte, void* va, void* arg)
        {
                page_t* page = ppn2page(PTE2PPN(*pte));
index ca4306c..9125aaa 100644 (file)
@@ -264,7 +264,7 @@ error_t page_free(page_t *SAFE page)
 }
 
 /*
- * Check if a page with the given pyhysical page # is free
+ * Check if a page with the given physical page # is free
  */
 int page_is_free(size_t ppn) {
        page_t* page = ppn2page(ppn);
index 876c6a7..386d2d4 100644 (file)
@@ -316,7 +316,7 @@ static void __proc_free(struct proc *p)
        }
 
        // Flush all mapped pages in the user portion of the address space
-       env_user_mem_free(p,0,KERNBASE);
+       env_user_mem_free(p, 0, UVPT);
        /* These need to be free again, since they were allocated with a refcnt. */
        free_cont_pages(p->env_procinfo, LOG2_UP(PROCINFO_NUM_PAGES));
        free_cont_pages(p->env_procdata, LOG2_UP(PROCDATA_NUM_PAGES));