Re: [ros-dev] Re: [ros-svn] [ion] 13832: - FreeLdr Part II (ntoskrnl is now relocated, removes 3GB compiler flag). Note that there is a bug in LD which Filip and I are examining, so do not try this yet.

[Hartmut Birr]

Hi,

I can fix my problem by protecting the access to some entries of the
object header with a global spinnlock, but I doesn't like this. Has
anyone a better idea?

- Hartmut

Hartmut Birr schrieb:
> Alex Ionescu schrieb:
> > James Tabor wrote:
> > > BugCheck Alert!
> >
> > Hmm...looks like after reverting Thomas's patch and using Hartmut's,
> > the problems are happening again.
> > So, it would seem that Thomas' patch is more correct? Or perhaps both
> > should be used?
> > T/H: Let me know!
>
> Hi,
>
> I've written a little test program which does always crash ros with my
> patch on the smp machine (up not tested). The program does create 10
> threads which try to open a non existing registry key in an endless
> loop. It does not crash with Thomas' patch. His patch adds one reference
> to much to the first parsed/opened key. This makes it impossible to
> delete a key and to unload the registry at shut down. I think we have a
> problem outside from the registry. It seems there is a gap between the
> deleting of an object after the last dereference operation and the next
> create operation for a new object with the same name. One thread has
> reopend the object and use it and the other delete the same object. I
> see very often values of 0xcccccxxx on bug checks and the wrong
> reference value -858993460 is also 0xcccccccc.
> Hartmut
>
> Ros-dev mailing list
> [e-mail]
> http://reactos.com:8080/mailman/listinfo/ros-dev

M:\Sandbox\ros_work\reactos>set SVN_EDITOR=notepad

M:\Sandbox\ros_work\reactos>d:\programme\subversion\bin\svn.exe diff ntoskrnl\ob\object.c        
Index: ntoskrnl/ob/object.c
===================================================================
--- ntoskrnl/ob/object.c	(revision 13857)
+++ ntoskrnl/ob/object.c	(working copy)
@@ -22,6 +22,7 @@
   POBJECT_HEADER ObjectHeader;
 } RETENTION_CHECK_PARAMS, *PRETENTION_CHECK_PARAMS;

+KSPIN_LOCK ObpLock = 0;

/* FUNCTIONS ************************************************************/

@@ -818,6 +819,8 @@
    		   IN KPROCESSOR_MODE AccessMode)
 {
    POBJECT_HEADER Header;
+   KIRQL oldIrql;
+   NTSTATUS Status;

/* NOTE: should be possible to reference an object above APC_LEVEL! */

@@ -849,22 +852,32 @@
    DPRINT("eip %x\n", ((PULONG)&Object)[-1]);
      }

-   if (Header->CloseInProcess)
-   {
-      if (Header->ObjectType == PsProcessType)
-        {
-	  return STATUS_PROCESS_IS_TERMINATING;
-	}
-      if (Header->ObjectType == PsThreadType)
-        {
-	  return STATUS_THREAD_IS_TERMINATING;
-	}
-      return(STATUS_UNSUCCESSFUL);
-   }
+   KeAcquireSpinLock(&ObpLock, &oldIrql);

-   InterlockedIncrement(&Header->RefCount);
+   if (Header->CloseInProcess ||
+       (0 == Header->RefCount && 0 == Header->HandleCount && !Header->Permanent))
+     {
+       if (Header->ObjectType == PsProcessType)
+         {
+	   Status = STATUS_PROCESS_IS_TERMINATING;
+	 }
+       else if (Header->ObjectType == PsThreadType)
+         {
+	   Status = STATUS_THREAD_IS_TERMINATING;
+	 }
+       else
+         {
+           Status = STATUS_UNSUCCESSFUL;
+         }
+     }
+   else
+     {
+       InterlockedIncrement(&Header->RefCount);
+       Status = STATUS_SUCCESS;
+     } 
+   KeReleaseSpinLock(&ObpLock, oldIrql);

-   return(STATUS_SUCCESS);
+   return Status;
 }

@@ -960,7 +973,7 @@

STATIC NTSTATUS
 ObpDeleteObjectDpcLevel(IN POBJECT_HEADER ObjectHeader,
-			IN LONG OldRefCount)
+			IN KIRQL oldIrql)
 {
   if (ObjectHeader->RefCount < 0)
     {
@@ -981,11 +994,14 @@
   if (ObjectHeader->CloseInProcess)
     {
       KEBUGCHECK(0);
+      KeReleaseSpinLock(&ObpLock, oldIrql);
       return STATUS_UNSUCCESSFUL;
     }
   ObjectHeader->CloseInProcess = TRUE;

-  switch (KeGetCurrentIrql ())
+  KeReleaseSpinLock(&ObpLock, oldIrql);
+  
+  switch (oldIrql)
     {
     case PASSIVE_LEVEL:
       return ObpDeleteObject (ObjectHeader);
@@ -1043,18 +1059,23 @@
 ObfReferenceObject(IN PVOID Object)
 {
   POBJECT_HEADER Header;
+  KIRQL oldIrql;

ASSERT(Object);

Header = BODY_TO_HEADER(Object);

+  KeAcquireSpinLock(&ObpLock, &oldIrql);
+
   /* No one should be referencing an object once we are deleting it. */
-  if (Header->CloseInProcess)
+  if (Header->CloseInProcess || 
+      (0 == Header->RefCount && 0 == Header->HandleCount && !Header->Permanent))
     {
       KEBUGCHECK(0);
     }

(VOID)InterlockedIncrement(&Header->RefCount);
+  KeReleaseSpinLock(&ObpLock, oldIrql);
 }

@@ -1081,9 +1102,12 @@
   LONG NewRefCount;
   BOOL Permanent;
   ULONG HandleCount;
+  KIRQL oldIrql;

ASSERT(Object);

+  KeAcquireSpinLock(&ObpLock, &oldIrql);
+
   /* Extract the object header. */
   Header = BODY_TO_HEADER(Object);
   Permanent = Header->Permanent;
@@ -1101,8 +1125,12 @@
       HandleCount == 0 &&
       !Permanent)
     {
-      ObpDeleteObjectDpcLevel(Header, NewRefCount);
+      ObpDeleteObjectDpcLevel(Header, oldIrql);
     }
+  else
+    {
+      KeReleaseSpinLock(&ObpLock, oldIrql);
+    }
 }