From 1809ae55e56053f12c5f54b9fc825b9fba90c358 Mon Sep 17 00:00:00 2001 From: Daniel Hammer Date: Sat, 28 Feb 2026 00:14:28 +0100 Subject: [PATCH] fix: continue fix page fault heap regression --- kernel/src/Api/Heap.hpp | 18 ++------ kernel/src/Api/Process.hpp | 30 ++++++++++++- kernel/src/Api/WinServer.cpp | 14 ++++++ kernel/src/Memory/PageFrameAllocator.cpp | 52 +++++++++++++++------- kernel/src/Memory/Paging.cpp | 54 +++++++++++++++++++++++ kernel/src/Memory/Paging.hpp | 5 +++ kernel/src/Sched/Scheduler.cpp | 35 +++++++++++++-- programs/src/fontpreview/main.cpp | 6 +-- ramdisk.tar | Bin 23060480 -> 23060480 bytes 9 files changed, 174 insertions(+), 40 deletions(-) diff --git a/kernel/src/Api/Heap.hpp b/kernel/src/Api/Heap.hpp index 6078c3f..986e96b 100644 --- a/kernel/src/Api/Heap.hpp +++ b/kernel/src/Api/Heap.hpp @@ -61,22 +61,10 @@ namespace Zenith { return userVa; } - // Free all heap allocations for a process slot (called on process exit) - static void CleanupHeapForSlot(int slot, uint64_t pml4Phys) { + // Reset heap allocation tracking for a process slot. + // The actual physical pages are freed by Paging::FreeUserHalf() during process cleanup. + static void CleanupHeapForSlot(int slot, uint64_t /*pml4Phys*/) { if (slot < 0 || slot >= Sched::MaxProcesses) return; - - for (int i = 0; i < g_heapAllocCount[slot]; i++) { - uint64_t va = g_heapAllocs[slot][i].va; - uint64_t numPages = g_heapAllocs[slot][i].numPages; - for (uint64_t p = 0; p < numPages; p++) { - uint64_t pageVa = va + p * 0x1000; - uint64_t physAddr = Memory::VMM::Paging::GetPhysAddr(pml4Phys, pageVa); - if (physAddr != 0) { - Memory::g_pfa->Free((void*)Memory::HHDM(physAddr)); - } - Memory::VMM::Paging::UnmapUserIn(pml4Phys, pageVa); - } - } g_heapAllocCount[slot] = 0; } diff --git a/kernel/src/Api/Process.hpp b/kernel/src/Api/Process.hpp index f595fba..36ddf1d 100644 --- a/kernel/src/Api/Process.hpp +++ b/kernel/src/Api/Process.hpp @@ -8,6 +8,9 @@ #pragma once #include #include +#include +#include +#include #include "Syscall.hpp" #include "WinServer.hpp" @@ -105,9 +108,34 @@ namespace Zenith { auto* proc = Sched::GetProcessByPid(pid); if (!proc) return -1; - // Clean up any windows owned by this process + // Clean up any windows owned by this process (unmaps pixel pages from desktop) WinServer::CleanupProcess(pid); + // Free I/O redirect buffers + if (proc->outBuf) { + Memory::g_pfa->Free(proc->outBuf); + proc->outBuf = nullptr; + } + if (proc->inBuf) { + Memory::g_pfa->Free(proc->inBuf); + proc->inBuf = nullptr; + } + + // Free all user-space pages and page table structures + Memory::VMM::Paging::FreeUserHalf(proc->pml4Phys); + + // Free kernel stack (safe — killed process isn't running on single-core) + if (proc->stackBase != 0) { + Memory::g_pfa->Free((void*)proc->stackBase, Sched::StackPages); + proc->stackBase = 0; + } + + // Free the PML4 page + if (proc->pml4Phys != 0) { + Memory::g_pfa->Free((void*)Memory::HHDM(proc->pml4Phys)); + proc->pml4Phys = 0; + } + proc->state = Sched::ProcessState::Terminated; return 0; } diff --git a/kernel/src/Api/WinServer.cpp b/kernel/src/Api/WinServer.cpp index 116ecf6..f5aed90 100644 --- a/kernel/src/Api/WinServer.cpp +++ b/kernel/src/Api/WinServer.cpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace WinServer { @@ -229,6 +230,19 @@ namespace WinServer { if (g_slots[i].used && g_slots[i].ownerPid == pid) { Kt::KernelLogStream(Kt::INFO, "WinServer") << "Cleaning up window " << i << " for exited PID " << pid; + + // Unmap pixel pages from desktop's address space to prevent stale access + if (g_slots[i].desktopVa != 0 && g_slots[i].desktopPid != 0) { + auto* desktopProc = Sched::GetProcessByPid(g_slots[i].desktopPid); + if (desktopProc) { + for (int p = 0; p < g_slots[i].pixelNumPages; p++) { + Memory::VMM::Paging::UnmapUserIn( + desktopProc->pml4Phys, + g_slots[i].desktopVa + (uint64_t)p * 0x1000); + } + } + } + g_slots[i].used = false; } } diff --git a/kernel/src/Memory/PageFrameAllocator.cpp b/kernel/src/Memory/PageFrameAllocator.cpp index 2aa86d4..e547da5 100644 --- a/kernel/src/Memory/PageFrameAllocator.cpp +++ b/kernel/src/Memory/PageFrameAllocator.cpp @@ -69,25 +69,43 @@ namespace Memory { } void* PageFrameAllocator::ReallocConsecutive(void* ptr, int n) { - auto first = Allocate(); + Lock.Acquire(); - for (int i = 0; i < n - 1; i++) { - if (Allocate() == nullptr) { - Panic("PageFrameAllocator Reallocation failed", nullptr); - }; + // Search the free list for a single contiguous region >= n pages. + // The old implementation assumed N consecutive Allocate() calls gave + // adjacent pages, which breaks when individual pages are freed back. + size_t needed = (size_t)n * 0x1000; + Page* current = head.next; + Page* prev = &head; + + while (current != nullptr) { + if (current->size >= needed) { + // Carve from the top of this contiguous region + current->size -= needed; + void* base; + if (current->size == 0) { + base = (void*)current; + prev->next = current->next; + } else { + base = (void*)((uint64_t)current + current->size); + } + + Lock.Release(); + + if (ptr != nullptr) { + memcpy(base, ptr, 0x1000); // copy one page (ptr is always a single page) + Free(ptr); + } + + return base; + } + prev = current; + current = current->next; } - // Allocate() returns pages from the top of a free region in descending - // order, so 'first' is the highest address. The contiguous block - // actually starts (n-1) pages below 'first'. - void* base = (void*)((uint64_t)first - (uint64_t)(n - 1) * 0x1000); - - if (ptr != nullptr) { - memcpy(base, ptr, (uint64_t)n * 0x1000); - Free(ptr); - } - - return base; + Lock.Release(); + Panic("PageFrameAllocator: no contiguous region available", nullptr); + return nullptr; } void PageFrameAllocator::Free(void* ptr) { @@ -102,7 +120,7 @@ namespace Memory { void PageFrameAllocator::Free(void* ptr, int n) { for (int i = 0; i < n; i++) { - Free((void*)(uint64_t)ptr + (0x1000 * n)); + Free((void*)((uint64_t)ptr + 0x1000 * i)); } } diff --git a/kernel/src/Memory/Paging.cpp b/kernel/src/Memory/Paging.cpp index 2520a0b..c62c2b3 100644 --- a/kernel/src/Memory/Paging.cpp +++ b/kernel/src/Memory/Paging.cpp @@ -268,6 +268,60 @@ namespace Memory::VMM { asm volatile("invlpg (%0)" :: "r"(virtualAddress) : "memory"); } + void Paging::FreeUserHalf(std::uint64_t pml4Phys) { + // PageTable* pointers store PHYSICAL addresses (same convention as MapUserIn/UnmapUserIn). + // Each individual entry access goes through HHDM() to get a valid virtual pointer. + PageTable* pml4 = (PageTable*)pml4Phys; + + // Walk user-half entries (0-255); kernel-half (256-511) is shared and must not be touched. + for (int i4 = 0; i4 < 256; i4++) { + PageTableEntry* pml4e = (PageTableEntry*)Memory::HHDM(&pml4->entries[i4]); + if (!pml4e->Present) continue; + + uint64_t pdptPhys = (uint64_t)pml4e->Address << 12; + PageTable* pdpt = (PageTable*)pdptPhys; + + for (int i3 = 0; i3 < 512; i3++) { + PageTableEntry* pdpte = (PageTableEntry*)Memory::HHDM(&pdpt->entries[i3]); + if (!pdpte->Present) continue; + + uint64_t pdPhys = (uint64_t)pdpte->Address << 12; + PageTable* pd = (PageTable*)pdPhys; + + for (int i2 = 0; i2 < 512; i2++) { + PageTableEntry* pde = (PageTableEntry*)Memory::HHDM(&pd->entries[i2]); + if (!pde->Present) continue; + + uint64_t ptPhys = (uint64_t)pde->Address << 12; + PageTable* pt = (PageTable*)ptPhys; + + // Free all leaf physical pages + for (int i1 = 0; i1 < 512; i1++) { + PageTableEntry* pte = (PageTableEntry*)Memory::HHDM(&pt->entries[i1]); + if (!pte->Present) continue; + + // Skip MMIO/WC pages (not PFA-managed) + if (pte->WriteThrough || pte->CacheDisabled) continue; + + uint64_t pagePhys = (uint64_t)pte->Address << 12; + if (pagePhys != 0) { + Memory::g_pfa->Free((void*)Memory::HHDM(pagePhys)); + } + } + + // Free the PT page itself + Memory::g_pfa->Free((void*)Memory::HHDM(ptPhys)); + } + + // Free the PD page + Memory::g_pfa->Free((void*)Memory::HHDM(pdPhys)); + } + + // Free the PDPT page + Memory::g_pfa->Free((void*)Memory::HHDM(pdptPhys)); + } + } + std::uint64_t Paging::GetPhysAddr(std::uint64_t pml4, std::uint64_t virtualAddress, bool use40BitL1) { VirtualAddress virtualAddressObj(virtualAddress); diff --git a/kernel/src/Memory/Paging.hpp b/kernel/src/Memory/Paging.hpp index 73b33b3..0b17f0b 100644 --- a/kernel/src/Memory/Paging.hpp +++ b/kernel/src/Memory/Paging.hpp @@ -112,6 +112,11 @@ public: // Unmap a single page from an arbitrary PML4 (clears PTE + invalidates TLB). static void UnmapUserIn(std::uint64_t pml4Phys, std::uint64_t virtualAddress); + // Free all user-half page table structures and physical pages (PML4 entries 0-255). + // Does NOT free the PML4 page itself (caller handles that). + // Skips MMIO/WC pages (WriteThrough or CacheDisabled set in PTE). + static void FreeUserHalf(std::uint64_t pml4Phys); + // Identity-map EFI runtime service regions so firmware code can // reference its own data at physical addresses. void MapEfiRuntime(limine_efi_memmap_response* efiMemmap); diff --git a/kernel/src/Sched/Scheduler.cpp b/kernel/src/Sched/Scheduler.cpp index a7574a7..f4a83be 100644 --- a/kernel/src/Sched/Scheduler.cpp +++ b/kernel/src/Sched/Scheduler.cpp @@ -239,9 +239,19 @@ namespace Sched { } void Schedule() { - // Reclaim terminated process slots so they can be reused + // Reclaim terminated process slots — free deferred resources first for (int i = 0; i < MaxProcesses; i++) { if (processTable[i].state == ProcessState::Terminated) { + // Free kernel stack (deferred from ExitProcess — can't free while running on it) + if (processTable[i].stackBase != 0) { + Memory::g_pfa->Free((void*)processTable[i].stackBase, StackPages); + processTable[i].stackBase = 0; + } + // Free the PML4 page (deferred from ExitProcess — can't free while it's CR3) + if (processTable[i].pml4Phys != 0) { + Memory::g_pfa->Free((void*)Memory::HHDM(processTable[i].pml4Phys)); + processTable[i].pml4Phys = 0; + } processTable[i].state = ProcessState::Free; } } @@ -322,10 +332,27 @@ namespace Sched { return; } - // Clean up any windows owned by this process - WinServer::CleanupProcess(processTable[currentPid].pid); + Process& proc = processTable[currentPid]; - processTable[currentPid].state = ProcessState::Terminated; + // Clean up any windows owned by this process (unmaps pixel pages from desktop) + WinServer::CleanupProcess(proc.pid); + + // Free I/O redirect buffers (kernel-allocated pages) + if (proc.outBuf) { + Memory::g_pfa->Free(proc.outBuf); + proc.outBuf = nullptr; + } + if (proc.inBuf) { + Memory::g_pfa->Free(proc.inBuf); + proc.inBuf = nullptr; + } + + // Free all user-space physical pages and page table structures (entries 0-255). + // This covers ELF code/data, user stack, exit stub, heap, and window pixel buffers. + // The PML4 page itself is freed later in Schedule() (can't free while it's CR3). + Memory::VMM::Paging::FreeUserHalf(proc.pml4Phys); + + proc.state = ProcessState::Terminated; int next = -1; for (int i = 0; i < MaxProcesses; i++) { diff --git a/programs/src/fontpreview/main.cpp b/programs/src/fontpreview/main.cpp index e8650b4..ba40a8e 100644 --- a/programs/src/fontpreview/main.cpp +++ b/programs/src/fontpreview/main.cpp @@ -1,6 +1,6 @@ /* * main.cpp - * ZenithOS Font Preview - standalone Window Server process + * ZenithOS Font Preview app * Displays TTF font samples at multiple sizes with vertical scrolling * Copyright (c) 2026 Daniel Hammer */ @@ -14,7 +14,7 @@ using namespace gui; // ============================================================================ -// Constants — light theme matching ZenithOS desktop conventions +// Constants // ============================================================================ static constexpr int INIT_W = 800; @@ -40,7 +40,7 @@ static const char* UPPER = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; static const char* LOWER_NUM = "abcdefghijklmnopqrstuvwxyz 0123456789"; // ============================================================================ -// Pre-rendered glyph cache — rasterised once at startup for all 7 sizes +// Pre-rendered glyph cache // ============================================================================ struct PreviewGlyph { diff --git a/ramdisk.tar b/ramdisk.tar index 824c7e6011fd88be8a10b60abbae9d7b1c5372c4..bd1347689f2d363ba1c07befc7e3b536d00f5080 100644 GIT binary patch delta 3796 zcmY+`cX$*<7zS|mLP#zG7g~;hlmswSsPQJZC#b|zIqY&O zh#~^Pib@zjBT7-RAQClp#DcwGi6APzUqW`j`{VaM56{ldeKYgzgoQ61&00tnDT@Ld z(w04;`Q*bdrjblc@FH(;VIM%Eg>0CmwFdfv^lWbgr21jnq}s^eUp|hm?5s>|E#q zUEz4>2Hl|t^n^UfhhESd`oIZL0DYk!^oJ8+0GtFT!$43$104)7!2%l`7zBf12n>Z` zFdRleA&i7kFdB+r44eX|!f7xTieVg#hY2tdCP4{IhAD75OoeH12Am0J!P#&QoC~Ee z9nOREp$sm73*jQT7-qmsm<5->rEnR{hB`3g= zrqeN7sG9BYwyKLH`uWcs&9a+&Nh)=bnTnkCU!)-08=)Jv;rJs$B9ZPt(^cLfAyKHv zkvU>wz#oC>Sr6rUZ_`JkYrJbpMMjTg<_J;cy{tKo|Gh}1$XynY%SVZWt(r#A6sl@i zesRSjk-?s!bpM&<-Vu;@P8I(gdm9_0tD2$uBT7Z0hxgk$9L$l9%9 zPfNd0)!?VQQzX*-nX$!e_k)#rX)^y!F^9kH_;&-H{~SeLzE|Y6_2)NChke`o`Pwjh zYHw@^$eQoO2glC8@qloK{@Q->)`n-e?i{u=#p32ba?&9&Ke^(t$dc)Y@Nd5gO|#fM zN4zQ;=dld7f5FPv(&Vm|l{?rzsHVkwI9SQsMAJ<6cClC&NUH1HZt~R*m9O{<(Hzre z{SdB91ij+Wrz%?r9dUeCex;n1A1#>9NAXW>LA3dxs1tRI7qL^+1mwo5X#KH+qK!nW zvZGuaP5FJpO!<8}#HG(rX^_l+shQT*~f3$h?8^1u6&s-{IE6KUl(E&2~eYBy-pF+1Bc77p2eWtm* z&v1&YOD(tgM;f=z;>WYv-4u|U3*C450(IVrMK0T1Tm)9i_LL}c@+5aSds{Odb~IC5 zc0+2mVY9l{M#puRs!$w5iL?3_Mn$9Z^VRMrkSGn~~0~%XDrSn#&6t3A8 zyBnJ>p7m})mOs6!^N%Vd#w*Ez)zP+cLXMcL$X&JW4z^0w;dlMZZlaS{1}<`bSr&^M z1Iab3#GvH*LN`mpw(@<9LC+G7RKRc||d@q>ABy5X4YEJI?fB8$hy#_+#oHgA=Z z7;j9);Dg2G&9~tb@nkad-lr zgs0$XSP##@v+x{jfEw5c5;nnR*aFYP3s4I$!d7?*UWRS39d^J@cm-aC*I*aC4sXDl z@D}Wbw_y*w1ACzk-i7zzeW-^I;6wNbK88==Q}_(_!RN3azJM>`EBG1?z&G$MdCdFA4XH%R*aV|wE#dM1ED9%^5DrIHL(S`p5g(+-W delta 3796 zcmZA3cXSk06b5i-LP$0O8(P+Ylmsw<2-62d2oO|43kcRAv7&;CL5=&evw})omBlWr zf+!*otf+*CB?1a6RwQ8TiUoVuh@#?m6EbtxKYrgiIrHYteeb@R&4SGbau$$<%0hL0 z#^S16)6sOpI7Bm$)*-#MROK7p&ymXT%seF#iDt-1v`y9X!6?nv{80&v!pOGC7yRqG z>u5$WOJyiesflFOM6&B5p{h&RFxNF(hb5HC5aht2&<@&5rLseEn?K&rY}21Mp}1kl zg^th(Izt!eDit>`S?}McyN2bLi#btFBGQxx5gBPZtZJS=($Fl^uZWf(RuZ|h^PwAb zhr^)<^n_l}8w#Kh`aoak2S-2=^oId35RQaFa1;!NA)tW{2AE)h4Gy?46o$dkFdUA7 z5ik;pVHAvpF;D_y;aE5hj)!qj3gck{OoT}=8OmS^OobC*8cc^1;UqX2PJvV5G$@A| za5~I{3OEDKgtOpmI0w#!S#Tbl4;R2}m;)EWMQ}09g-f6k=E0?K8C(ukaD~jB9gUyw zcSmZb9Gy5Jxfw)h@Pv+S^q|ha$82|INGLTvJqjC%nYuf@$7_zMX@4B z<%vmZFhaL&HX`4@n_e12=clGzWc5mAj}$emI9IcS;!3=ws$4c&{M^Pw%qx#)<)4r8a6Lto2XOehMHI-Ym{!dtd)ym zY0o#UwC7V3m&6JVzQoO2ly;ie#FBmdx(rQs*an2f235+}V{fyUSW7#EZ>3ePn=Wf` zeskrGvBvCRe$(WezbD2wKmGkJ7qgYr+}hY6nffNyR1!>K*e+XO%Q@3Me$LQEc0O#6 zKSZ}&w(+aHjjDXA*xMB}*U$_;e~H(+xy`M?;RzZa7G;W@GT9r!wnw)d)|#na>lczX z-ZA<2Z%u4`PdVXoZ_L4OD7_(g#o664FWE0xw~j-@pIxV>Hmvj-TLv`u)!_qH`Gq%6 z?C|r}^5I$M73BoeYlg*o7Zwwg)R5X(2RSiM%vI#}dT%S=BfJ4YEnB=~SHBEHw>0*0 zl!|NA)asRDXlh-tmo2xx>uu-XE8Ak9?2prTOS9NK4bh>B9Jbe+-X$p6(D`od=XV>r zt+VCq5mJ>KGvd418g$!a&(4Y`+xu(4SA_rAtPok5sqAj?v|l4$c=veg5u-zbe+1d) z> z6<&ka;SG2bcEDS(6B=L_?1s0Y5#E7!;XQaCK7bG5Bls9Tfj#gkdH6o*mdQ*@)~PH{Lz4~m`? zy(oH96i^gW^r7fW(U0N?iXw{s6ay#*QXEM!h~g-U!4yL%Gzy)sQB0wj pN^t_kG>YjICsLe5aWcgz6sJ<0Mo~^NgW`0GnaU=mqCz>a;6D>;YHk1k