16 Dec
2007
16 Dec
'07
4:37 p.m.
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?…
==============================================================================
--- 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?…
==============================================================================
--- 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/…
==============================================================================
--- 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/…
==============================================================================
--- 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