Thomas Weidenmueller schrieb:
I believe this is due to a bug in the object parsing code where it doesn't reference an object when walking into deeper levels. Could you please try my attached patch on SVN trunk if it fixes it?
Best Regards, Thomas
Hi,
I think your patch isn't correct. The parsing routine from an object must always reference the returned object because an other thread may try to remove this object. Since a long time I'm searching for a bug which corrupts the registry and which crashs ros on my smp machine. It was always triggered by the font substitution query routine from win32k. I've added a missing locking operation and moved the referencing into the locked region. This fixes my smp crash and may also fix James problem. My test condition is compiling ros on ros in one console and running ctm in an other one.
- Hartmut
M:\Sandbox\ros_work\reactos>set SVN_EDITOR=notepad
M:\Sandbox\ros_work\reactos>d:\programme\subversion\bin\svn.exe diff ntoskrnl\cm Index: ntoskrnl/cm/registry.c =================================================================== --- ntoskrnl/cm/registry.c (revision 13819) +++ ntoskrnl/cm/registry.c (working copy) @@ -20,7 +20,6 @@
POBJECT_TYPE CmiKeyType = NULL; PREGISTRY_HIVE CmiVolatileHive = NULL; -KSPIN_LOCK CmiKeyListLock;
LIST_ENTRY CmiHiveListHead;
@@ -336,7 +335,6 @@ CmiVolatileHive->RootSecurityCell = RootSecurityCell; #endif
- KeInitializeSpinLock(&CmiKeyListLock);
/* Create '\Registry\Machine' key. */ RtlInitUnicodeString(&KeyName, Index: ntoskrnl/cm/regobj.c =================================================================== --- ntoskrnl/cm/regobj.c (revision 13819) +++ ntoskrnl/cm/regobj.c (working copy) @@ -76,6 +76,9 @@ KeyName.Length); KeyName.Buffer[KeyName.Length / sizeof(WCHAR)] = 0;
+ /* Acquire hive lock */ + KeEnterCriticalRegion(); + ExAcquireResourceExclusiveLite(&CmiRegistryLock, TRUE);
FoundObject = CmiScanKeyList(ParsedKey, &KeyName, @@ -91,6 +94,8 @@ Attributes); if (!NT_SUCCESS(Status) || (SubKeyCell == NULL)) { + ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion(); RtlFreeUnicodeString(&KeyName); return(STATUS_UNSUCCESSFUL); } @@ -104,6 +109,9 @@ &LinkPath); if (NT_SUCCESS(Status)) { + ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion(); + DPRINT("LinkPath '%wZ'\n", &LinkPath);
/* build new FullPath for reparsing */ @@ -152,6 +160,8 @@ (PVOID*)&FoundObject); if (!NT_SUCCESS(Status)) { + ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion(); RtlFreeUnicodeString(&KeyName); return(Status); } @@ -179,7 +189,12 @@ if (NT_SUCCESS(Status)) { DPRINT("LinkPath '%wZ'\n", &LinkPath); + + ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion();
+ ObDereferenceObject(FoundObject); + /* build new FullPath for reparsing */ TargetPath.MaximumLength = LinkPath.MaximumLength; if (EndPtr != NULL) @@ -212,12 +227,9 @@ return(STATUS_REPARSE); } } - - ObReferenceObjectByPointer(FoundObject, - STANDARD_RIGHTS_REQUIRED, - NULL, - UserMode); } + ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion();
DPRINT("CmiObjectParse: %s\n", FoundObject->Name);
@@ -274,6 +286,10 @@
ObReferenceObject (ParentKeyObject);
+ /* Acquire hive lock */ + KeEnterCriticalRegion(); + ExAcquireResourceExclusiveLite(&CmiRegistryLock, TRUE); + if (!NT_SUCCESS(CmiRemoveKeyFromList(KeyObject))) { DPRINT1("Key not found in parent list ???\n"); @@ -302,6 +318,9 @@
ObDereferenceObject (ParentKeyObject);
+ ExReleaseResourceLite(&CmiRegistryLock); + KeLeaveCriticalRegion(); + if (KeyObject->NumberOfSubKeys) { KEBUGCHECK(REGISTRY_ERROR); @@ -532,11 +551,9 @@ CmiAddKeyToList(PKEY_OBJECT ParentKey, PKEY_OBJECT NewKey) { - KIRQL OldIrql;
DPRINT("ParentKey %.08x\n", ParentKey);
- KeAcquireSpinLock(&CmiKeyListLock, &OldIrql);
if (ParentKey->SizeOfSubKeys <= ParentKey->NumberOfSubKeys) { @@ -568,7 +585,6 @@ NULL, UserMode); NewKey->ParentKey = ParentKey; - KeReleaseSpinLock(&CmiKeyListLock, OldIrql); }
@@ -576,11 +592,9 @@ CmiRemoveKeyFromList(PKEY_OBJECT KeyToRemove) { PKEY_OBJECT ParentKey; - KIRQL OldIrql; DWORD Index;
ParentKey = KeyToRemove->ParentKey; - KeAcquireSpinLock(&CmiKeyListLock, &OldIrql); /* FIXME: If list maintained in alphabetic order, use dichotomic search */ for (Index = 0; Index < ParentKey->NumberOfSubKeys; Index++) { @@ -591,7 +605,6 @@ &ParentKey->SubKeys[Index + 1], (ParentKey->NumberOfSubKeys - Index - 1) * sizeof(PKEY_OBJECT)); ParentKey->NumberOfSubKeys--; - KeReleaseSpinLock(&CmiKeyListLock, OldIrql);
DPRINT("Dereference parent key: 0x%x\n", ParentKey); @@ -599,7 +612,6 @@ return STATUS_SUCCESS; } } - KeReleaseSpinLock(&CmiKeyListLock, OldIrql);
return STATUS_UNSUCCESSFUL; } @@ -611,13 +623,12 @@ ULONG Attributes) { PKEY_OBJECT CurKey; - KIRQL OldIrql; ULONG Index; - + NTSTATUS Status; + DPRINT("Scanning key list for: %wZ (Parent: %wZ)\n", KeyName, &Parent->Name);
- KeAcquireSpinLock(&CmiKeyListLock, &OldIrql); /* FIXME: if list maintained in alphabetic order, use dichotomic search */ for (Index=0; Index < Parent->NumberOfSubKeys; Index++) { @@ -627,8 +638,7 @@ if ((KeyName->Length == CurKey->Name.Length) && (_wcsicmp(KeyName->Buffer, CurKey->Name.Buffer) == 0)) { - KeReleaseSpinLock(&CmiKeyListLock, OldIrql); - return CurKey; + break; } } else @@ -636,13 +646,23 @@ if ((KeyName->Length == CurKey->Name.Length) && (wcscmp(KeyName->Buffer, CurKey->Name.Buffer) == 0)) { - KeReleaseSpinLock(&CmiKeyListLock, OldIrql); - return CurKey; + break; } } } - KeReleaseSpinLock(&CmiKeyListLock, OldIrql);
+ if (Index < Parent->NumberOfSubKeys) + { + Status = ObReferenceObjectByPointer(CurKey, + STANDARD_RIGHTS_REQUIRED, + NULL, + UserMode); + if (NT_SUCCESS(Status)) + { + return CurKey; + } + } + return NULL; }