From 10614e04929a6d109a5e90524e446b637aaf0c13 Mon Sep 17 00:00:00 2001 From: Richard Qin Date: Sat, 3 Jan 2026 02:13:08 +0800 Subject: Fix: critical malloc size calculation bug Summary: - Fixed an integer arithmetic error in malloc() where 'nunits' was initialized to 0, causing incorrect heap sizing. - This bug caused malloc to return pointers pointing to unmapped memory pages, leading to Store Page Fault (scause 15) at process boundaries. - Switched memory allocator logic to use byte-based size calculation instead of K&R style units to prevent off-by-one errors. Improvements: - Shell now starts successfully without segfaults. --- src/kernel/mm/vm.cc | 3 -- src/kernel/proc/exec.cc | 10 ++++-- src/kernel/proc/proc.cc | 83 +++++++++++++++++++++++++++++++++------------- src/kernel/trap/handler.cc | 18 ++++++---- 4 files changed, 78 insertions(+), 36 deletions(-) (limited to 'src/kernel') diff --git a/src/kernel/mm/vm.cc b/src/kernel/mm/vm.cc index 41d1aa1..1155d2c 100644 --- a/src/kernel/mm/vm.cc +++ b/src/kernel/mm/vm.cc @@ -5,9 +5,6 @@ #include "lib/string.h" #include "drivers/uart.h" -// COW Flag (PTE bit 8) -#define PTE_COW (1L << 8) - extern "C" { extern char kernel_end[]; diff --git a/src/kernel/proc/exec.cc b/src/kernel/proc/exec.cc index d0e75ef..22e4842 100644 --- a/src/kernel/proc/exec.cc +++ b/src/kernel/proc/exec.cc @@ -129,15 +129,19 @@ namespace Exec // Prepare user stack uint64 sz = PGROUNDUP(vm_guard.sz); - uint64 sz1 = VM::uvmalloc(vm_guard.pagetable, sz, sz + 2 * PGSIZE, PTE_W | PTE_R | PTE_U); + + const int stack_data_pages = 16; + const int stack_total_pages = stack_data_pages + 1; + + uint64 sz1 = VM::uvmalloc(vm_guard.pagetable, sz, sz + stack_total_pages * PGSIZE, PTE_W | PTE_R | PTE_U); if (sz1 == 0) return -1; vm_guard.sz = sz1; - VM::uvmunmap(vm_guard.pagetable, sz1 - 2 * PGSIZE, 1, 0); // protected pagetable + VM::uvmunmap(vm_guard.pagetable, sz, 1, 0); // protected pagetable uint64 sp = sz1; - uint64 stackbase = sp - PGSIZE; + uint64 stackbase = sp - stack_data_pages * PGSIZE; uint64 ustack[32]; uint64 argc = 0; diff --git a/src/kernel/proc/proc.cc b/src/kernel/proc/proc.cc index f1f1c1d..5db5f7a 100644 --- a/src/kernel/proc/proc.cc +++ b/src/kernel/proc/proc.cc @@ -7,12 +7,13 @@ #include "kernel/mm.h" #include "kernel/trap.h" #include "kernel/riscv.h" +#include "kernel/slab.h" #include "drivers/uart.h" #include "fs/fs.h" #include "fs/file.h" #include "lib/string.h" -#define NPROC 64 +// #define NPROC 64 #define NCPU 8 extern "C" void swtch(struct Context *old_ctx, struct Context *new_ctx); @@ -21,7 +22,9 @@ extern "C" void usertrap(); extern "C" void userret(uint64, uint64); extern char initcode_start[], initcode_end[]; -struct Proc procs[NPROC]; +// struct Proc procs[NPROC]; +struct Proc *proc_list_head = nullptr; +struct Proc *initproc = nullptr; struct Spinlock proc_mem_lock; struct cpu cpus[NCPU]; struct RunQueue runqueues[NCPU]; @@ -161,15 +164,24 @@ static Proc *allocproc() { struct Proc *p; proc_mem_lock.acquire(); - for (p = procs; p < &procs[NPROC]; p++) + + p = (struct Proc *)Slab::kmalloc(sizeof(struct Proc)); + + if (!p) { - if (p->state == UNUSED) - goto found; + proc_mem_lock.release(); + return 0; } - proc_mem_lock.release(); - return 0; -found: + memset(p, 0, sizeof(struct Proc)); + + p->all_next = proc_list_head; + p->all_prev = nullptr; + if (proc_list_head) + proc_list_head->all_prev = p; + proc_list_head = p; + + p->state = USED; p->pid = nextpid++; p->priority = 0; @@ -181,13 +193,29 @@ found: if ((p->tf = (struct Trapframe *)PMM::alloc_page()) == 0) { p->state = UNUSED; + if (p->all_next) + p->all_next->all_prev = p->all_prev; + if (p->all_prev) + p->all_prev->all_next = p->all_next; + if (p == proc_list_head) + proc_list_head = p->all_next; + Slab::kfree(p); proc_mem_lock.release(); return 0; } + if ((p->kstack = PMM::alloc_page()) == 0) { PMM::free_page(p->tf); p->state = UNUSED; + if (p->all_next) + p->all_next->all_prev = p->all_prev; + if (p->all_prev) + p->all_prev->all_next = p->all_next; + if (p == proc_list_head) + proc_list_head = p->all_next; + Slab::kfree(p); + proc_mem_lock.release(); return 0; } @@ -224,13 +252,16 @@ static void freeproc(struct Proc *p) VFS::iput(p->cwd); p->cwd = nullptr; } - p->pagetable = 0; - p->sz = 0; - p->pid = 0; - p->parent = 0; - p->name[0] = 0; - p->chan = 0; - p->state = UNUSED; + + if (p->all_prev) + p->all_prev->all_next = p->all_next; + else + proc_list_head = p->all_next; + + if (p->all_next) + p->all_next->all_prev = p->all_prev; + + Slab::kfree(p); } int fork() @@ -242,13 +273,14 @@ int fork() return -1; np->pagetable = VM::uvmcreate(); + np->sz = p->sz; if (VM::uvmcopy(p->pagetable, np->pagetable, p->sz) < 0) { freeproc(np); proc_mem_lock.release(); return -1; } - np->sz = p->sz; + if (VM::mappages(np->pagetable, TRAPFRAME, PGSIZE, (uint64)np->tf, PTE_R | PTE_W) < 0) { @@ -293,8 +325,8 @@ namespace ProcManager void init() { proc_mem_lock.init("proc_mem"); - for (int i = 0; i < NPROC; i++) - procs[i].state = UNUSED; + + proc_list_head = nullptr; for (int i = 0; i < NCPU; i++) { @@ -373,6 +405,8 @@ namespace ProcManager if (!p) return; + initproc = p; + p->pagetable = VM::uvmcreate(); uint64 initcode_sz = initcode_end - initcode_start; @@ -460,7 +494,7 @@ namespace ProcManager void wakeup(void *chan) { - for (struct Proc *p = procs; p < &procs[NPROC]; p++) + for (struct Proc *p = proc_list_head; p != nullptr; p = p->all_next) { if (p->state == SLEEPING && p->chan == chan) { @@ -476,14 +510,15 @@ namespace ProcManager void exit(int status) { struct Proc *p = myproc(); - struct Proc *initproc = &procs[0]; if (p == initproc) while (1) ; wakeup(p->parent); - for (struct Proc *pp = procs; pp < &procs[NPROC]; pp++) + proc_mem_lock.acquire(); + + for (struct Proc *pp = proc_list_head; pp != nullptr; pp = pp->all_next) { if (pp->parent == p) { @@ -492,6 +527,8 @@ namespace ProcManager } } + proc_mem_lock.release(); + p->xstate = status; struct cpu *c = mycpu(); @@ -513,7 +550,7 @@ namespace ProcManager havekids = 0; proc_mem_lock.acquire(); - for (np = procs; np < &procs[NPROC]; np++) + for (np = proc_list_head; np != nullptr; np = np->all_next) { if (np->parent == p) { @@ -561,7 +598,7 @@ namespace ProcManager { // Call VM::uvmalloc to allocate memory // Parameters: page table, old size, new size, permissions (PTE_W) - if ((sz = VM::uvmalloc(p->pagetable, sz, sz + n, PTE_W)) == 0) + if ((sz = VM::uvmalloc(p->pagetable, sz, sz + n, PTE_W | PTE_R | PTE_U)) == 0) { return -1; } diff --git a/src/kernel/trap/handler.cc b/src/kernel/trap/handler.cc index 3e73213..3fb772e 100644 --- a/src/kernel/trap/handler.cc +++ b/src/kernel/trap/handler.cc @@ -157,15 +157,19 @@ extern "C" void usertrap() } else if (scause == 15) // Store Page Fault { - uint64 pte = VM::walkaddr(p->pagetable, stval); - - // Drivers::uart_puts("[Debug] Faulting PTE:"); - // Drivers::print_hex(pte); - // Drivers::uart_puts("\n"); - if (VM::handle_cow_fault(p->pagetable, PGROUNDDOWN(stval)) != 0) { - Drivers::uart_puts("usertrap: cow failed at "); + uint64 pa = VM::walkaddr(p->pagetable, stval); + + if (pa == 0) + { + Drivers::uart_puts("usertrap: Segmentation Fault (unmapped) at "); + } + else + { + Drivers::uart_puts("usertrap: Protection Fault (or OOM) at "); + } + Drivers::print_hex(stval); Drivers::uart_puts("\n"); p->state = ZOMBIE; -- cgit v1.2.3