[ion] 55666: [NTOSKRNL]: This is kind of embarssing, but after doing a local grep for some of cgutman and zekflop's changes (respectively the patch that had added MmDeleteProcessPageDirectory, and ...
Author: ion Date: Fri Feb 17 07:07:47 2012 New Revision: 55666
URL: http://svn.reactos.org/svn/reactos?rev=55666&view=rev Log: [NTOSKRNL]: This is kind of embarssing, but after doing a local grep for some of cgutman and zekflop's changes (respectively the patch that had added MmDeleteProcessPageDirectory, and the mshtml fix patch), I fell upon a .patch file. It included 3 memory-leak-fixing patches from richard that had been sent to me during his last days -- which, had I paid attention, would've fixed the MSHTML bug and the memory leaks months ago! I've tried my best to now re-integrate a portion of the patch (the other two portions deal with freeing the loader block, and freeing .INIT sections, and I will commit them later) with the current state of trunk. Some things that patch did, no longer seem to work, and I've commented where appropriate. But in general it does seem to dereference/delete some PTEs that had been left behind before (such as deleted TEBs, and deleted VA mappings). It no longer seems to be able to delete the root PDE nor the shared data page however (this worked in the original patch's timeframe). Zekflop, tkreuzer, please take a look. I hope this will set us on a better path!
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c trunk/reactos/ntoskrnl/mm/ARM3/virtual.c trunk/reactos/ntoskrnl/mm/amd64/page.c trunk/reactos/ntoskrnl/mm/i386/page.c trunk/reactos/ntoskrnl/mm/marea.c
Modified: trunk/reactos/ntoskrnl/mm/ARM3/procsup.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/procsup.c?... ============================================================================== --- trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/ARM3/procsup.c [iso-8859-1] Fri Feb 17 07:07:47 2012 @@ -199,6 +199,9 @@ ASSERT(VadTree->NumberGenericTableElements >= 1); MiRemoveNode((PMMADDRESS_NODE)Vad, VadTree);
+ /* Delete the pages */ + MiDeleteVirtualAddresses((ULONG_PTR)Teb, TebEnd, NULL); + /* Release the working set */ MiUnlockProcessWorkingSet(Process, Thread);
@@ -219,7 +222,7 @@ IN BOOLEAN GuiStack) { PMMPTE PointerPte; - PFN_NUMBER PageFrameNumber;//, PageTableFrameNumber; + PFN_NUMBER PageFrameNumber, PageTableFrameNumber; PFN_COUNT StackPages; PMMPFN Pfn1;//, Pfn2; ULONG i; @@ -253,13 +256,14 @@ /* Get the PTE's page */ PageFrameNumber = PFN_FROM_PTE(PointerPte); Pfn1 = MiGetPfnEntry(PageFrameNumber); -#if 0 // ARM3 might not own the page table, so don't take this risk. Leak it instead! +#if 1 // ARM3 might not own the page table, so don't take this risk. Leak it instead! /* Now get the page of the page table mapping it */ PageTableFrameNumber = Pfn1->u4.PteFrame; - Pfn2 = MiGetPfnEntry(PageTableFrameNumber); + //Pfn2 = MiGetPfnEntry(PageTableFrameNumber);
/* Remove a shared reference, since the page is going away */ - MiDecrementShareCount(Pfn2, PageTableFrameNumber); + DPRINT("SystemPTE PDE: %lx\n", PageTableFrameNumber); + //MiDecrementShareCount(Pfn2, PageTableFrameNumber); #endif /* Set the special pending delete marker */ MI_SET_PFN_DELETED(Pfn1); @@ -1340,6 +1344,102 @@ MmUnlockAddressSpace(&Process->Vm); }
+VOID +NTAPI +MmDeleteProcessAddressSpace2(IN PEPROCESS Process) +{ + PMMPFN Pfn1, Pfn2; + KIRQL OldIrql; + PFN_NUMBER PageFrameIndex; + PULONG PageDir; + ULONG i; + + //ASSERT(Process->CommitCharge == 0); + + /* Delete the shared user data section (Should be done in clean, not delete) */ + ASSERT(MmHighestUserAddress > (PVOID)USER_SHARED_DATA); + KeAttachProcess(&Process->Pcb); + //DPRINT1("Killing shared user data page no longer works -- has someone changed ARM3 in a way to make this fail now?\n"); + //MiDeleteVirtualAddresses(USER_SHARED_DATA, USER_SHARED_DATA, NULL); + //DPRINT1("Done\n"); + KeDetachProcess(); + + /* Acquire the PFN lock */ + OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock); + + /* Check for fully initialized process */ + if (Process->AddressSpaceInitialized == 2) + { + /* Map the PDE */ + PageDir = MmCreateHyperspaceMapping(Process->Pcb.DirectoryTableBase[0] >> PAGE_SHIFT); + for (i = 0; i < ADDR_TO_PDE_OFFSET(MmSystemRangeStart); i++) + { + /* Loop all page tables */ + if (PageDir[i] != 0) + { + /* Check if they are RosMm-owned */ + if (MI_IS_ROS_PFN(MiGetPfnEntry(PageDir[i] >> PAGE_SHIFT))) + { + /* Free them through the RosMm ABI */ + //DPRINT1("Freeing MC_SYSTEM: %lx\n", PageDir[i]); + MmReleasePageMemoryConsumer(MC_SYSTEM, PageDir[i] >> PAGE_SHIFT); + } + } + } + + /* Unmap the PDE */ + //DPRINT1("Done\n"); + MmDeleteHyperspaceMapping(PageDir); + + /* Map the working set page and its page table */ + Pfn1 = MiGetPfnEntry(Process->WorkingSetPage); + Pfn2 = MiGetPfnEntry(Pfn1->u4.PteFrame); + + /* Nuke it */ + MI_SET_PFN_DELETED(Pfn1); + MiDecrementShareCount(Pfn2, Pfn1->u4.PteFrame); + MiDecrementShareCount(Pfn1, Process->WorkingSetPage); + ASSERT((Pfn1->u3.e2.ReferenceCount == 0) || (Pfn1->u3.e1.WriteInProgress)); + + /* Now map hyperspace and its page table */ + PageFrameIndex = Process->Pcb.DirectoryTableBase[1] >> PAGE_SHIFT; + Pfn1 = MiGetPfnEntry(PageFrameIndex); + Pfn2 = MiGetPfnEntry(Pfn1->u4.PteFrame); + + /* Nuke it */ + MI_SET_PFN_DELETED(Pfn1); + MiDecrementShareCount(Pfn2, Pfn1->u4.PteFrame); + MiDecrementShareCount(Pfn1, PageFrameIndex); + ASSERT((Pfn1->u3.e2.ReferenceCount == 0) || (Pfn1->u3.e1.WriteInProgress)); + + /* Finally, nuke the PDE itself */ + PageFrameIndex = Process->Pcb.DirectoryTableBase[0] >> PAGE_SHIFT; + Pfn1 = MiGetPfnEntry(PageFrameIndex); + MI_SET_PFN_DELETED(Pfn1); + MiDecrementShareCount(Pfn1, PageFrameIndex); + MiDecrementShareCount(Pfn1, PageFrameIndex); + + /* HACK: In Richard's original patch this ASSERT did work */ + //DPRINT1("Ref count: %lx %lx\n", Pfn1->u3.e2.ReferenceCount, Pfn1->u2.ShareCount); + //ASSERT((Pfn1->u3.e2.ReferenceCount == 0) || (Pfn1->u3.e1.WriteInProgress)); + } + else + { + /* A partly-initialized process should never exit through here */ + ASSERT(FALSE); + } + + /* Release the PFN lock */ + KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql); + + /* No support for sessions yet */ + ASSERT(Process->Session == 0); + + /* Clear out the PDE pages */ + Process->Pcb.DirectoryTableBase[0] = 0; + Process->Pcb.DirectoryTableBase[1] = 0; +} + /* SESSION CODE TO MOVE TO SESSION.C ******************************************/
KGUARDED_MUTEX MiSessionIdMutex;
Modified: trunk/reactos/ntoskrnl/mm/ARM3/virtual.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/ARM3/virtual.c?... ============================================================================== --- trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/ARM3/virtual.c [iso-8859-1] Fri Feb 17 07:07:47 2012 @@ -264,8 +264,21 @@ /* Drop the share count */ MiDecrementShareCount(Pfn1, PageFrameIndex);
- /* No fork yet */ - if (PointerPte <= MiHighestUserPte) ASSERT(PrototypePte == Pfn1->PteAddress); + /* Either a fork, or this is the shared user data page */ + if (PointerPte <= MiHighestUserPte) + { + /* If it's not the shared user page, then crash, since there's no fork() yet */ + if ((PAGE_ALIGN(VirtualAddress) != (PVOID)USER_SHARED_DATA) || + (MmHighestUserAddress <= (PVOID)USER_SHARED_DATA)) + { + /* Must be some sort of memory corruption */ + KeBugCheckEx(MEMORY_MANAGEMENT, + 0x400, + (ULONG_PTR)PointerPte, + (ULONG_PTR)PrototypePte, + (ULONG_PTR)Pfn1->PteAddress); + } + } } else { @@ -284,7 +297,8 @@ ASSERT(Pfn1->u2.ShareCount == 1);
/* FIXME: Drop the reference on the page table. For now, leak it until RosMM is gone */ - //MiDecrementShareCount(MiGetPfnEntry(Pfn1->u4.PteFrame), Pfn1->u4.PteFrame); + //DPRINT1("Dropping a ref...\n"); + MiDecrementShareCount(MiGetPfnEntry(Pfn1->u4.PteFrame), Pfn1->u4.PteFrame);
/* Mark the PFN for deletion and dereference what should be the last ref */ MI_SET_PFN_DELETED(Pfn1); @@ -312,6 +326,7 @@ KIRQL OldIrql; BOOLEAN AddressGap = FALSE; PSUBSECTION Subsection; + PUSHORT UsedPageTableEntries;
/* Get out if this is a fake VAD, RosMm will free the marea pages */ if ((Vad) && (Vad->u.VadFlags.Spare == 1)) return; @@ -368,6 +383,7 @@ /* Now we should have a valid PDE, mapped in, and still have some VA */ ASSERT(PointerPde->u.Hard.Valid == 1); ASSERT(Va <= EndingAddress); + UsedPageTableEntries = &MmWorkingSetList->UsedPageTableEntries[MiGetPdeOffset(Va)];
/* Check if this is a section VAD with gaps in it */ if ((AddressGap) && (LastPrototypePte)) @@ -397,6 +413,11 @@ TempPte = *PointerPte; if (TempPte.u.Long) { + DPRINT("Decrement used PTEs by address: %lx\n", Va); + (*UsedPageTableEntries)--; + ASSERT((*UsedPageTableEntries) < PTE_COUNT); + DPRINT("Refs: %lx\n", (*UsedPageTableEntries)); + /* Check if the PTE is actually mapped in */ if (TempPte.u.Long & 0xFFFFFC01) { @@ -456,6 +477,22 @@ /* The PDE should still be valid at this point */ ASSERT(PointerPde->u.Hard.Valid == 1);
+ DPRINT("Should check if handles for: %p are zero (PDE: %lx)\n", Va, PointerPde->u.Hard.PageFrameNumber); + if (!(*UsedPageTableEntries)) + { + DPRINT("They are!\n"); + if (PointerPde->u.Long != 0) + { + DPRINT("PDE active: %lx in %16s\n", PointerPde->u.Hard.PageFrameNumber, CurrentProcess->ImageFileName); + + /* Delete the PTE proper */ + MiDeletePte(PointerPde, + MiPteToAddress(PointerPde), + CurrentProcess, + NULL); + } + } + /* Release the lock and get out if we're done */ KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql); if (Va > EndingAddress) return;
Modified: trunk/reactos/ntoskrnl/mm/amd64/page.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/amd64/page.c?re... ============================================================================== --- trunk/reactos/ntoskrnl/mm/amd64/page.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/amd64/page.c [iso-8859-1] Fri Feb 17 07:07:47 2012 @@ -188,29 +188,6 @@ } }
-VOID -NTAPI -MmDeleteProcessPageDirectory(PEPROCESS Process) -{ - PFN_NUMBER TableBasePfn; - PMMPTE PageDir; - - /* Get the page directory PFN */ - TableBasePfn = Process->Pcb.DirectoryTableBase[0] >> PAGE_SHIFT; - - /* Map the page directory in hyperspace */ - PageDir = (PMMPTE)MmCreateHyperspaceMapping(TableBasePfn); - - /* Free the hyperspace mapping page (ARM3) */ - //MmDeletePageTablePfn(PageDir[ADDR_TO_PDE_OFFSET(HYPERSPACE)].u.Hard.PageFrameNumber, 3); - - /* Delete the hyperspace mapping */ - MmDeleteHyperspaceMapping(PageDir); - - /* Recursively free the page directories */ - MmDeletePageTablePfn(TableBasePfn, 4); -} - static PMMPTE MiGetPteForProcess(
Modified: trunk/reactos/ntoskrnl/mm/i386/page.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/i386/page.c?rev... ============================================================================== --- trunk/reactos/ntoskrnl/mm/i386/page.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/i386/page.c [iso-8859-1] Fri Feb 17 07:07:47 2012 @@ -198,62 +198,6 @@ return(Attributes); }
-VOID -FORCEINLINE -MmDeletePageDirectoryEntry(PMMPDE PointerPde) -{ - KIRQL OldIrql; - PMMPFN Page; - - Page = MiGetPfnEntry(PointerPde->u.Hard.PageFrameNumber); - - /* This should not be a legacy allocation*/ - ASSERT(MI_IS_ROS_PFN(Page) == FALSE); - - OldIrql = KeAcquireQueuedSpinLock(LockQueuePfnLock); - - /* Free it using the ARM3 API */ - MI_SET_PFN_DELETED(Page); - MiDecrementShareCount(Page, PointerPde->u.Hard.PageFrameNumber); - - KeReleaseQueuedSpinLock(LockQueuePfnLock, OldIrql); -} - -VOID -NTAPI -MmDeleteProcessPageDirectory(PEPROCESS Process) -{ - PMMPDE PdeBase, PointerPde; - ULONG PdeOffset; - KIRQL OldIrql; - - /* Map the page directory in hyperspace */ - PdeBase = MiMapPageInHyperSpace(PsGetCurrentProcess(), - Process->Pcb.DirectoryTableBase[0] >> PAGE_SHIFT, - &OldIrql); - - /* Loop the user land page directory */ - for (PdeOffset = 0; PdeOffset < MiGetPdeOffset(MmSystemRangeStart); PdeOffset++) - { - PointerPde = PdeBase + PdeOffset; - /* Check if a valid PDE exists here */ - if (PointerPde->u.Hard.Valid) - { - /* Free the page that backs it */ - MmDeletePageDirectoryEntry(PointerPde); - } - } - - /* Free the hyperspace mapping page (ARM3) */ - MmDeletePageDirectoryEntry(PdeBase + MiGetPdeOffset(HYPERSPACE)); - - /* Delete the hyperspace mapping */ - MiUnmapPageInHyperSpace(PsGetCurrentProcess(), PdeBase, OldIrql); - - /* Free the PDE page itself (ARM3) */ - MmDeletePageDirectoryEntry((PMMPDE)&Process->Pcb.DirectoryTableBase[0]); -} - /* Taken from ARM3/pagfault.c */ BOOLEAN FORCEINLINE
Modified: trunk/reactos/ntoskrnl/mm/marea.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/mm/marea.c?rev=556... ============================================================================== --- trunk/reactos/ntoskrnl/mm/marea.c [iso-8859-1] (original) +++ trunk/reactos/ntoskrnl/mm/marea.c [iso-8859-1] Fri Feb 17 07:07:47 2012 @@ -994,6 +994,10 @@ } }
+VOID +NTAPI +MmDeleteProcessAddressSpace2(IN PEPROCESS Process); + NTSTATUS NTAPI MmDeleteProcessAddressSpace(PEPROCESS Process) @@ -1036,11 +1040,10 @@ } }
- MmDeleteProcessPageDirectory(Process); - MmUnlockAddressSpace(&Process->Vm);
DPRINT("Finished MmReleaseMmInfo()\n"); + MmDeleteProcessAddressSpace2(Process); return(STATUS_SUCCESS); }
-
ion＠svn.reactos.org