Re: [ros-dev] Alex has fixed the laptop problem

[Hartmut Birr]

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;
 }