Author: fireball Date: Sun Dec 16 19:37:15 2007 New Revision: 31282
URL: http://svn.reactos.org/svn/reactos?rev=31282&view=rev Log: - Add a signature field to KCBs, and use it to validate KCBs as well as to mark them dead after they've been freed (to catch invalid reuse). - Add assertions to make sure key hashes are valid. - Finish implementing EnlistKeyBodyWithKCB since the new parse routine is now used, and implement analogous DelistKeyBodyFromKCB. - Fix delay item allocation code which was overwriting pool memory. - Re-enable allocation-page support for allocating KCBs. - Implement the CmpDelayDerefKCBWorker now that delay alloc/free works. - Fix CmpDelayDerefKeyControlBlock which was overwriting KCB flags and incorrectly setting reference counts for the KCB. - CmpDelayCloseWorkItemActive should be enabled when arming the delayed close timer. - Fix reference counting bugs in CmpAddToDelayedClose and CmpRemoveFromDelayedClose. - Fix wrong sizes of CM_KCBS_PER_PAGE and CM_DELAYS_PER_PAGE which caused more pool overwrites.
Modified: trunk/reactos/ntoskrnl/config/cmalloc.c trunk/reactos/ntoskrnl/config/cmdelay.c trunk/reactos/ntoskrnl/config/cmkcbncb.c trunk/reactos/ntoskrnl/config/cmsysini.c trunk/reactos/ntoskrnl/include/internal/cm.h trunk/reactos/ntoskrnl/include/internal/cm_x.h
Modified: trunk/reactos/ntoskrnl/config/cmalloc.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmalloc.c?r... ============================================================================== --- trunk/reactos/ntoskrnl/config/cmalloc.c (original) +++ trunk/reactos/ntoskrnl/config/cmalloc.c Sun Dec 16 19:37:15 2007 @@ -55,7 +55,7 @@ PAGED_CODE();
/* Sanity checks */ - ASSERT(IsListEmpty(&(Kcb->KeyBodyListHead)) == TRUE); + ASSERT(IsListEmpty(&Kcb->KeyBodyListHead) == TRUE); for (i = 0; i < 4; i++) ASSERT(Kcb->KeyBodyArray[i] == NULL);
/* Check if it wasn't privately allocated */ @@ -87,12 +87,12 @@ if (++AllocPage->FreeCount == CM_KCBS_PER_PAGE) { /* Loop all the entries */ - for (i = CM_KCBS_PER_PAGE; i; i--) + for (i = 0; i < CM_KCBS_PER_PAGE; i++) { /* Get the KCB */ Kcb = (PVOID)((ULONG_PTR)AllocPage + FIELD_OFFSET(CM_ALLOC_PAGE, AllocPage) + - (i - 1) * sizeof(CM_KEY_CONTROL_BLOCK)); + i * sizeof(CM_KEY_CONTROL_BLOCK));
/* Remove the entry */ RemoveEntryList(&Kcb->FreeListEntry); @@ -117,7 +117,7 @@ PAGED_CODE();
/* Check if private allocations are initialized */ - if (FALSE) + if (CmpAllocInited) { /* They are, acquire the bucket lock */ KeAcquireGuardedMutex(&CmpAllocBucketLock); @@ -208,28 +208,28 @@
/* Look for an item on the free list */ SearchList: - if (!IsListEmpty(&CmpFreeDelayItemsListHead)) - { - /* Get the current entry in the list */ - NextEntry = RemoveHeadList(&CmpFreeDelayItemsListHead); - - /* Grab the item */ - Entry = CONTAINING_RECORD(NextEntry, CM_DELAY_ALLOC, ListEntry); - - /* Clear the list */ - Entry->ListEntry.Flink = Entry->ListEntry.Blink = NULL; - - /* Grab the alloc page */ - AllocPage = (PCM_ALLOC_PAGE)((ULONG_PTR)Entry & 0xFFFFF000); - - /* Decrease free entries */ - ASSERT(AllocPage->FreeCount != 0); - AllocPage->FreeCount--; - - /* Release the lock */ - KeReleaseGuardedMutex(&CmpDelayAllocBucketLock); - return Entry; - } + if (!IsListEmpty(&CmpFreeDelayItemsListHead)) + { + /* Get the current entry in the list */ + NextEntry = RemoveHeadList(&CmpFreeDelayItemsListHead); + + /* Grab the item */ + Entry = CONTAINING_RECORD(NextEntry, CM_DELAY_ALLOC, ListEntry); + + /* Clear the list */ + Entry->ListEntry.Flink = Entry->ListEntry.Blink = NULL; + + /* Grab the alloc page */ + AllocPage = (PCM_ALLOC_PAGE)((ULONG_PTR)Entry & 0xFFFFF000); + + /* Decrease free entries */ + ASSERT(AllocPage->FreeCount != 0); + AllocPage->FreeCount--; + + /* Release the lock */ + KeReleaseGuardedMutex(&CmpDelayAllocBucketLock); + return Entry; + }
/* Allocate an allocation page */ AllocPage = ExAllocatePoolWithTag(PagedPool, PAGE_SIZE, TAG_CM);
Modified: trunk/reactos/ntoskrnl/config/cmdelay.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmdelay.c?r... ============================================================================== --- trunk/reactos/ntoskrnl/config/cmdelay.c (original) +++ trunk/reactos/ntoskrnl/config/cmdelay.c Sun Dec 16 19:37:15 2007 @@ -100,15 +100,41 @@ NTAPI CmpDelayDerefKCBWorker(IN PVOID Context) { + PCM_DELAY_DEREF_KCB_ITEM Entry; PAGED_CODE();
/* Sanity check */ ASSERT(CmpDelayDerefKCBWorkItemActive);
- /* FIXME: TODO */ - DPRINT1("CmpDelayDerefKCBWorker has work to do!\n"); - return; - ASSERT(FALSE); + /* Lock the registry and and list lock */ + CmpLockRegistry(); + KeAcquireGuardedMutex(&CmpDelayDerefKCBLock); + + /* Check if the list is empty */ + while (!IsListEmpty(&CmpDelayDerefKCBListHead)) + { + /* Grab an entry */ + Entry = (PVOID)RemoveHeadList(&CmpDelayDerefKCBListHead); + + /* We can release the lock now */ + KeReleaseGuardedMutex(&CmpDelayDerefKCBLock); + + /* Now grab the actual entry */ + Entry = CONTAINING_RECORD(Entry, CM_DELAY_DEREF_KCB_ITEM, ListEntry); + Entry->ListEntry.Flink = Entry->ListEntry.Blink = NULL; + + /* Dereference and free */ + CmpDereferenceKeyControlBlock(Entry->Kcb); + CmpFreeDelayItem(Entry); + + /* Lock the list again */ + KeAcquireGuardedMutex(&CmpDelayDerefKCBLock); + } + + /* We're done */ + CmpDelayDerefKCBWorkItemActive = FALSE; + KeReleaseGuardedMutex(&CmpDelayDerefKCBLock); + CmpUnlockRegistry(); }
VOID @@ -133,19 +159,22 @@ NTAPI CmpDelayDerefKeyControlBlock(IN PCM_KEY_CONTROL_BLOCK Kcb) { - USHORT OldRefCount, NewRefCount; + LONG OldRefCount, NewRefCount; LARGE_INTEGER Timeout; PCM_DELAY_DEREF_KCB_ITEM Entry; PAGED_CODE();
/* Get the previous reference count */ - OldRefCount = Kcb->RefCount; - - /* Write the new one */ - NewRefCount = (USHORT)InterlockedCompareExchange((PLONG)&Kcb->RefCount, - OldRefCount - 1, - OldRefCount); - if (NewRefCount != OldRefCount) return; + OldRefCount = *(PLONG)&Kcb->RefCount; + NewRefCount = OldRefCount - 1; + if (((NewRefCount & 0xFFFF) > 0) && + (InterlockedCompareExchange((PLONG)&Kcb->RefCount, + NewRefCount, + OldRefCount) == OldRefCount)) + { + /* KCB still had references, so we're done */ + return; + }
/* Allocate a delay item */ Entry = CmpAllocateDelayItem(); @@ -179,6 +208,9 @@ { LARGE_INTEGER Timeout; PAGED_CODE(); + + /* Set the worker active */ + CmpDelayCloseWorkItemActive = TRUE;
/* Setup the interval */ Timeout.QuadPart = CmpDelayCloseIntervalInSeconds * -10000000; @@ -220,14 +252,18 @@ if (Kcb->InDelayClose) ASSERT(FALSE);
/* Get the previous reference count */ - OldRefCount = Kcb->InDelayClose; + OldRefCount = *(PLONG)&Kcb->InDelayClose; ASSERT(OldRefCount == 0);
/* Write the new one */ - NewRefCount = InterlockedCompareExchange((PLONG)&Kcb->InDelayClose, - 1, - OldRefCount); - if (NewRefCount != OldRefCount) ASSERT(FALSE); + NewRefCount = 1; + if (InterlockedCompareExchange((PLONG)&Kcb->InDelayClose, + NewRefCount, + OldRefCount) != OldRefCount) + { + /* Sanity check */ + ASSERT(FALSE); + }
/* Reset the delayed close index */ Kcb->DelayedCloseIndex = 0; @@ -290,15 +326,19 @@ /* Sanity check */ if (!Kcb->InDelayClose) ASSERT(FALSE);
- /* Get the old reference count */ - OldRefCount = Kcb->InDelayClose; + /* Get the previous reference count */ + OldRefCount = *(PLONG)&Kcb->InDelayClose; ASSERT(OldRefCount == 1);
- /* Set it to 0 */ - NewRefCount = InterlockedCompareExchange((PLONG)&Kcb->InDelayClose, - 0, - OldRefCount); - if (NewRefCount != OldRefCount) ASSERT(FALSE); + /* Write the new one */ + NewRefCount = 0; + if (InterlockedCompareExchange((PLONG)&Kcb->InDelayClose, + NewRefCount, + OldRefCount) != OldRefCount) + { + /* Sanity check */ + ASSERT(FALSE); + }
/* Remove the link to the entry */ Kcb->DelayCloseEntry = NULL; @@ -307,5 +347,3 @@ Kcb->DelayedCloseIndex = CmpDelayedCloseSize; }
- -
Modified: trunk/reactos/ntoskrnl/config/cmkcbncb.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmkcbncb.c?... ============================================================================== --- trunk/reactos/ntoskrnl/config/cmkcbncb.c (original) +++ trunk/reactos/ntoskrnl/config/cmkcbncb.c Sun Dec 16 19:37:15 2007 @@ -80,6 +80,7 @@ { PCM_KEY_HASH *Prev; PCM_KEY_HASH Current; + ASSERT_VALID_HASH(KeyHash);
/* Lookup all the keys in this index entry */ Prev = &GET_HASH_ENTRY(CmpCacheTable, KeyHash->ConvKey).Entry; @@ -88,12 +89,14 @@ /* Save the current one and make sure it's valid */ Current = *Prev; ASSERT(Current != NULL); + ASSERT_VALID_HASH(Current);
/* Check if it matches */ if (Current == KeyHash) { /* Then write the previous one */ *Prev = Current->NextHash; + if (*Prev) ASSERT_VALID_HASH(*Prev); break; }
@@ -109,6 +112,7 @@ { ULONG i; PCM_KEY_HASH Entry; + ASSERT_VALID_HASH(KeyHash);
/* Get the hash index */ i = GET_HASH_INDEX(KeyHash->ConvKey); @@ -121,6 +125,7 @@ while (Entry) { /* Check if this matches */ + ASSERT_VALID_HASH(Entry); if ((KeyHash->ConvKey == Entry->ConvKey) && (KeyHash->KeyCell == Entry->KeyCell) && (KeyHash->KeyHive == Entry->KeyHive)) @@ -488,6 +493,9 @@ Parent = Kcb->ParentKcb; if (!Kcb->Delete) CmpRemoveKeyControlBlock(Kcb);
+ /* Set invalid KCB signature */ + Kcb->Signature = CM_KCB_INVALID_SIGNATURE; + /* Free the KCB as well */ CmpFreeKeyControlBlock(Kcb);
@@ -595,12 +603,15 @@ IN BOOLEAN LockHeldExclusively) { /* Sanity check */ - ASSERT((CmpIsKcbLockedExclusive(Kcb) == TRUE) || - (CmpTestRegistryLockExclusive() == TRUE)); + ASSERT_KCB_VALID(Kcb);
/* Check if this is the last reference */ if ((InterlockedDecrement((PLONG)&Kcb->RefCount) & 0xFFFF) == 0) { + /* Sanity check */ + ASSERT((CmpIsKcbLockedExclusive(Kcb) == TRUE) || + (CmpTestRegistryLockExclusive() == TRUE)); + /* Check if we should do a direct delete */ if (((CmpHoldLazyFlush) && !(Kcb->ExtFlags & CM_KCB_SYM_LINK_FOUND) && @@ -699,6 +710,7 @@ InitializeKCBKeyBodyList(Kcb);
/* Set it up */ + Kcb->Signature = CM_KCB_SIGNATURE; Kcb->Delete = FALSE; Kcb->RefCount = 1; Kcb->KeyHive = Hive; @@ -706,6 +718,7 @@ Kcb->ConvKey = ConvKey; Kcb->DelayedCloseIndex = CmpDelayedCloseSize; Kcb->InDelayClose = 0; + ASSERT_KCB_VALID(Kcb);
/* Check if we have two hash entires */ HashLock = Flags & CMP_LOCK_HASHES_FOR_KCB ? TRUE : FALSE; @@ -730,9 +743,11 @@ { /* Sanity check */ ASSERT(!FoundKcb->Delete); + Kcb->Signature = CM_KCB_INVALID_SIGNATURE;
/* Free the one we allocated and reference this one */ CmpFreeKeyControlBlock(Kcb); + ASSERT_KCB_VALID(FoundKcb); Kcb = FoundKcb; if (!CmpReferenceKeyControlBlock(Kcb)) { @@ -790,6 +805,7 @@ { /* Remove the KCB and free it */ CmpRemoveKeyControlBlock(Kcb); + Kcb->Signature = CM_KCB_INVALID_SIGNATURE; CmpFreeKeyControlBlock(Kcb); Kcb = NULL; } @@ -833,12 +849,20 @@
/* Remove the KCB and free it */ CmpRemoveKeyControlBlock(Kcb); + Kcb->Signature = CM_KCB_INVALID_SIGNATURE; CmpFreeKeyControlBlock(Kcb); Kcb = NULL; } } }
+ /* Check if this is a KCB inside a frozen hive */ + if ((Kcb) && (((PCMHIVE)Hive)->Frozen) && (!(Kcb->Flags & KEY_SYM_LINK))) + { + /* Don't add these to the delay close */ + Kcb->ExtFlags |= CM_KCB_NO_DELAY_CLOSE; + } + /* Sanity check */ ASSERT((!Kcb) || (Kcb->Delete == FALSE));
@@ -867,12 +891,98 @@ EnlistKeyBodyWithKCB(IN PCM_KEY_BODY KeyBody, IN ULONG Flags) { + ULONG i; + /* Sanity check */ ASSERT(KeyBody->KeyControlBlock != NULL);
/* Initialize the list entry */ InitializeListHead(&KeyBody->KeyBodyList);
- /* FIXME: Implement once we don't link parents to children anymore */ -} - + /* Check if we can use the parent KCB array */ + for (i = 0; i < 4; i++) + { + /* Add it into the list */ + if (!InterlockedCompareExchangePointer(&KeyBody->KeyControlBlock-> + KeyBodyArray[i], + KeyBody, + NULL)) + { + /* Added */ + return; + } + } + + /* Array full, check if we need to unlock the KCB */ + if (Flags & CMP_ENLIST_KCB_LOCKED_SHARED) + { + /* It's shared, so release the KCB shared lock */ + CmpReleaseKcbLock(KeyBody->KeyControlBlock); + ASSERT(!(Flags & CMP_ENLIST_KCB_LOCKED_EXCLUSIVE)); + } + + /* Check if we need to lock the KCB */ + if (!(Flags & CMP_ENLIST_KCB_LOCKED_EXCLUSIVE)) + { + /* Acquire the lock */ + CmpAcquireKcbLockExclusive(KeyBody->KeyControlBlock); + } + + /* Make sure we have the exclusive lock */ + ASSERT((CmpIsKcbLockedExclusive(KeyBody->KeyControlBlock) == TRUE) || + (CmpTestRegistryLockExclusive() == TRUE)); + + /* do the insert */ + InsertTailList(&KeyBody->KeyControlBlock->KeyBodyListHead, + &KeyBody->KeyBodyList); + + /* Check if we did a manual lock */ + if (!(Flags & (CMP_ENLIST_KCB_LOCKED_SHARED | + CMP_ENLIST_KCB_LOCKED_EXCLUSIVE))) + { + /* Release the lock */ + CmpReleaseKcbLock(KeyBody->KeyControlBlock); + } +} + +VOID +NTAPI +DelistKeyBodyFromKCB(IN PCM_KEY_BODY KeyBody, + IN BOOLEAN LockHeld) +{ + ULONG i; + + /* Sanity check */ + ASSERT(KeyBody->KeyControlBlock != NULL); + + /* Check if we can use the parent KCB array */ + for (i = 0; i < 4; i++) + { + /* Add it into the list */ + if (InterlockedCompareExchangePointer(&KeyBody->KeyControlBlock-> + KeyBodyArray[i], + NULL, + KeyBody) == KeyBody) + { + /* Removed */ + return; + } + } + + /* Sanity checks */ + ASSERT(IsListEmpty(&KeyBody->KeyControlBlock->KeyBodyListHead) == FALSE); + ASSERT(IsListEmpty(&KeyBody->KeyBodyList) == FALSE); + + /* Lock the KCB */ + if (!LockHeld) CmpAcquireKcbLockExclusive(KeyBody->KeyControlBlock); + ASSERT((CmpIsKcbLockedExclusive(KeyBody->KeyControlBlock) == TRUE) || + (CmpTestRegistryLockExclusive() == TRUE)); + + /* Remove the entry */ + RemoveEntryList(&KeyBody->KeyBodyList); + + /* Unlock it it if we did a manual lock */ + if (!LockHeld) CmpReleaseKcbLock(KeyBody->KeyControlBlock); +} + +
Modified: trunk/reactos/ntoskrnl/config/cmsysini.c URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmsysini.c?... ============================================================================== --- trunk/reactos/ntoskrnl/config/cmsysini.c (original) +++ trunk/reactos/ntoskrnl/config/cmsysini.c Sun Dec 16 19:37:15 2007 @@ -66,8 +66,8 @@ Kcb = KeyBody->KeyControlBlock; if (Kcb) { - /* Delist the key (once new parse routines are used) */ - //DelistKeyBodyFromKCB(KeyBody, FALSE); + /* Delist the key */ + DelistKeyBodyFromKCB(KeyBody, FALSE); }
/* Dereference the KCB */
Modified: trunk/reactos/ntoskrnl/include/internal/cm.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/c... ============================================================================== --- trunk/reactos/ntoskrnl/include/internal/cm.h (original) +++ trunk/reactos/ntoskrnl/include/internal/cm.h Sun Dec 16 19:37:15 2007 @@ -36,12 +36,17 @@ #define CMTRACE(x, ...) DPRINT(__VA_ARGS__) #endif
- // // Hack since bigkeys are not yet supported // #define ASSERT_VALUE_BIG(h, s) \ ASSERTMSG("Big keys not supported!", !CmpIsKeyValueBig(h, s)); + +// +// CM_KEY_CONTROL_BLOCK Signatures +// +#define CM_KCB_SIGNATURE TAG('C', 'm', 'K', 'b') +#define CM_KCB_INVALID_SIGNATURE TAG('C', 'm', 'F', '4')
// // CM_KEY_CONTROL_BLOCK Flags @@ -81,6 +86,12 @@ #define CMP_LOCK_HASHES_FOR_KCB 0x2
// +// EnlistKeyBodyWithKCB Flags +// +#define CMP_ENLIST_KCB_LOCKED_SHARED 0x1 +#define CMP_ENLIST_KCB_LOCKED_EXCLUSIVE 0x2 + +// // Maximum size of Value Cache // #define MAXIMUM_CACHED_DATA 2 * PAGE_SIZE @@ -94,9 +105,9 @@ // Number of items that can fit inside an Allocation Page // #define CM_KCBS_PER_PAGE \ - PAGE_SIZE / sizeof(CM_KEY_CONTROL_BLOCK) + ((PAGE_SIZE - FIELD_OFFSET(CM_ALLOC_PAGE, AllocPage)) / sizeof(CM_KEY_CONTROL_BLOCK)) #define CM_DELAYS_PER_PAGE \ - PAGE_SIZE / sizeof(CM_DELAYED_CLOSE_ENTRY) + ((PAGE_SIZE - FIELD_OFFSET(CM_ALLOC_PAGE, AllocPage)) / sizeof(CM_DELAY_ALLOC))
// // Value Search Results @@ -229,6 +240,7 @@ // typedef struct _CM_KEY_CONTROL_BLOCK { + ULONG Signature; USHORT RefCount; USHORT Flags; struct @@ -905,6 +917,13 @@ EnlistKeyBodyWithKCB( IN PCM_KEY_BODY KeyObject, IN ULONG Flags +); + +VOID +NTAPI +DelistKeyBodyFromKCB( + IN PCM_KEY_BODY KeyBody, + IN BOOLEAN LockHeld );
NTSTATUS
Modified: trunk/reactos/ntoskrnl/include/internal/cm_x.h URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/c... ============================================================================== --- trunk/reactos/ntoskrnl/include/internal/cm_x.h (original) +++ trunk/reactos/ntoskrnl/include/internal/cm_x.h Sun Dec 16 19:37:15 2007 @@ -63,6 +63,8 @@ GET_HASH_KEY(ConvKey) % CmpHashTableSize #define GET_HASH_ENTRY(Table, ConvKey) \ (Table[GET_HASH_INDEX(ConvKey)]) +#define ASSERT_VALID_HASH(h) \ + ASSERT_KCB_VALID(CONTAINING_RECORD((h), CM_KEY_CONTROL_BLOCK, KeyHash))
// // Returns whether or not the cell is cached @@ -95,6 +97,12 @@ #define CMP_ASSERT_EXCLUSIVE_REGISTRY_LOCK() \ ASSERT((CmpSpecialBootCondition == TRUE) || \ (CmpTestRegistryLockExclusive() == TRUE)) + +// +// Makes sure this is a valid KCB +// +#define ASSERT_KCB_VALID(k) \ + ASSERT((k)->Signature == CM_KCB_SIGNATURE)
// // Checks if a KCB is exclusively locked