[ros-diffs] [ion] 25408: - Implement ObReferenceProcessHandleTable and ObDereferenceProcessHandleTable and use them where appropriate to avoid race issues if the process is being killed meanwhile. - Implement ObpReferenceProcessObjectByHandle and simplfy ObDuplicateObject. - Disable hard errors while closing handles, and protect against races. Also print our error message since it seems handles aren't being closed now (message displays leak count). - Honour DUPLICATE_CLOSE_SOURCE during failure paths in ObD

Author: ion
Date: Wed Jan 10 06:35:59 2007
New Revision: 25408

URL: http://svn.reactos.org/svn/reactos?rev=25408&view=rev
Log:
- Implement ObReferenceProcessHandleTable and ObDereferenceProcessHandleTable and use them
where appropriate to avoid race issues if the process is being killed meanwhile.
- Implement ObpReferenceProcessObjectByHandle and simplfy ObDuplicateObject.
- Disable hard errors while closing handles, and protect against races. Also print our
error message since it seems handles aren't being closed now (message displays leak
count).
- Honour DUPLICATE_CLOSE_SOURCE during failure paths in ObDuplicateObject, and catch race
conditions.
- Add some more sanity checks and speed up some internal referencing.

Modified:
    trunk/reactos/ntoskrnl/KrnlFun.c
    trunk/reactos/ntoskrnl/include/internal/ob.h
    trunk/reactos/ntoskrnl/ob/obhandle.c
    trunk/reactos/ntoskrnl/ob/obref.c

Modified: trunk/reactos/ntoskrnl/KrnlFun.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/KrnlFun.c?rev=254…
==============================================================================
--- trunk/reactos/ntoskrnl/KrnlFun.c (original)
+++ trunk/reactos/ntoskrnl/KrnlFun.c Wed Jan 10 06:35:59 2007
@@ -7,11 +7,6 @@
 //                        Do NOT complain about it.
 //                     Do NOT ask when it will be fixed.
 //              Failure to respect this will *ACHIEVE NOTHING*.
-//
-//
-// Ob:
-//  - Add Directory Lock.
-//  - Add Object Table Referencing.
 //
 // Ex:
 //  - Fixup existing code that talks to Ke.

Modified: trunk/reactos/ntoskrnl/include/internal/ob.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/include/internal/…
==============================================================================
--- trunk/reactos/ntoskrnl/include/internal/ob.h (original)
+++ trunk/reactos/ntoskrnl/include/internal/ob.h Wed Jan 10 06:35:59 2007
@@ -171,6 +171,18 @@
     IN PEPROCESS Process
 );
 
+PHANDLE_TABLE
+NTAPI
+ObReferenceProcessHandleTable(
+    IN PEPROCESS Process
+);
+
+VOID
+NTAPI
+ObDereferenceProcessHandleTable(
+    IN PEPROCESS Process
+);
+
 VOID
 NTAPI
 ObKillProcess(

Modified: trunk/reactos/ntoskrnl/ob/obhandle.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ob/obhandle.c?rev…
==============================================================================
--- trunk/reactos/ntoskrnl/ob/obhandle.c (original)
+++ trunk/reactos/ntoskrnl/ob/obhandle.c Wed Jan 10 06:35:59 2007
@@ -22,6 +22,165 @@
 #define TAG_OB_HANDLE TAG('O', 'b', 'H', 'd')
 
 /* PRIVATE FUNCTIONS *********************************************************/
+
+PHANDLE_TABLE
+NTAPI
+ObReferenceProcessHandleTable(IN PEPROCESS Process)
+{
+    PHANDLE_TABLE HandleTable = NULL;
+
+    /* Lock the process */
+    if (ExAcquireRundownProtection(&Process->RundownProtect))
+    {
+        /* Get the handle table */
+        HandleTable = Process->ObjectTable;
+        if (!HandleTable)
+        {
+            /* No table, release the lock */
+            ExReleaseRundownProtection(&Process->RundownProtect);
+        }
+    }
+
+    /* Return the handle table */
+    return HandleTable;
+}
+
+VOID
+NTAPI
+ObDereferenceProcessHandleTable(IN PEPROCESS Process)
+{
+    /* Release the process lock */
+    ExReleaseRundownProtection(&Process->RundownProtect);
+}
+
+NTSTATUS
+NTAPI
+ObpReferenceProcessObjectByHandle(IN HANDLE Handle,
+                                  IN PEPROCESS Process,
+                                  IN PHANDLE_TABLE HandleTable,
+                                  IN KPROCESSOR_MODE AccessMode,
+                                  OUT PVOID *Object,
+                                  OUT POBJECT_HANDLE_INFORMATION HandleInformation)
+{
+    PHANDLE_TABLE_ENTRY HandleEntry;
+    POBJECT_HEADER ObjectHeader;
+    ACCESS_MASK GrantedAccess;
+    ULONG Attributes;
+    PEPROCESS CurrentProcess;
+    PETHREAD CurrentThread;
+    NTSTATUS Status;
+    PAGED_CODE();
+
+    /* Assume failure */
+    *Object = NULL;
+
+    /* Check if the caller wants the current process */
+    if (Handle == NtCurrentProcess())
+    {
+        /* Get the current process */
+        CurrentProcess = PsGetCurrentProcess();
+
+        /* Check if the caller wanted handle information */
+        if (HandleInformation)
+        {
+            /* Return it */
+            HandleInformation->HandleAttributes = 0;
+            HandleInformation->GrantedAccess = Process->GrantedAccess;
+        }
+
+        /* Reference ourselves */
+        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentProcess);
+        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+
+        /* Return the pointer */
+        *Object = CurrentProcess;
+        return STATUS_SUCCESS;
+    }
+
+    /* Check if the caller wants the current thread */
+    if (Handle == NtCurrentThread())
+    {
+        /* Get the current thread */
+        CurrentThread = PsGetCurrentThread();
+
+        /* Check if the caller wanted handle information */
+        if (HandleInformation)
+        {
+            /* Return it */
+            HandleInformation->HandleAttributes = 0;
+            HandleInformation->GrantedAccess = CurrentThread->GrantedAccess;
+        }
+
+        /* Reference ourselves */
+        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentThread);
+        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+
+        /* Return the pointer */
+        *Object = CurrentThread;
+        return STATUS_SUCCESS;
+    }
+
+    /* Check if this is a kernel handle */
+    if (ObIsKernelHandle(Handle, AccessMode))
+    {
+        /* Use the kernel handle table and get the actual handle value */
+        Handle = ObKernelHandleToHandle(Handle);
+        HandleTable = ObpKernelHandleTable;
+    }
+    else
+    {
+        /* Otherwise use this process's handle table */
+        HandleTable = PsGetCurrentProcess()->ObjectTable;
+    }
+
+    /* Enter a critical region while we touch the handle table */
+    ASSERT(HandleTable != NULL);
+    KeEnterCriticalRegion();
+
+    /* Get the handle entry */
+    HandleEntry = ExMapHandleToPointer(HandleTable, Handle);
+    if (HandleEntry)
+    {
+        /* Get the object header and validate the type*/
+        ObjectHeader = EX_HTE_TO_HDR(HandleEntry);
+
+        /* Get the granted access and validate it */
+        GrantedAccess = HandleEntry->GrantedAccess;
+
+        /* Mask out the internal attributes */
+        Attributes = HandleEntry->ObAttributes &
+                     (EX_HANDLE_ENTRY_PROTECTFROMCLOSE |
+                      EX_HANDLE_ENTRY_INHERITABLE |
+                      EX_HANDLE_ENTRY_AUDITONCLOSE);
+
+        /* Fill out the information */
+        HandleInformation->HandleAttributes = Attributes;
+        HandleInformation->GrantedAccess = GrantedAccess;
+
+        /* Return the pointer */
+        *Object = &ObjectHeader->Body;
+
+        /* Add a reference */
+        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+
+        /* Unlock the handle */
+        ExUnlockHandleTableEntry(HandleTable, HandleEntry);
+        KeLeaveCriticalRegion();
+
+        /* Return success */
+        ASSERT(*Object != NULL);
+        return STATUS_SUCCESS;
+    }
+    else
+    {
+        /* Invalid handle */
+        Status = STATUS_INVALID_HANDLE;
+    }
+
+    /* Return failure status */
+    KeLeaveCriticalRegion();
+    return Status;
+}
 
 BOOLEAN
 NTAPI
@@ -1741,11 +1900,15 @@
     /* Check if we have a parent */
     if (Parent)
     {
+        /* Get the parent's table */
+        HandleTable = ObReferenceProcessHandleTable(Parent);
+        if (!HandleTable) return STATUS_PROCESS_IS_TERMINATING;
+
         /* Duplicate the parent's */
         HandleTable = ExDupHandleTable(Process,
                                        ObpDuplicateHandleCallback,
                                        NULL,
-                                       Parent->ObjectTable);
+                                       HandleTable);
     }
     else
     {
@@ -1753,11 +1916,14 @@
         HandleTable = ExCreateHandleTable(Process);
     }
 
-    /* Now write it and make sure we got one */
+    /* Now write it */
     Process->ObjectTable = HandleTable;
+
+    /* Dereference the parent's handle table if we have one */
+    if (Parent) ObDereferenceProcessHandleTable(Parent);
+
+    /* Fail or succeed depending on whether we got a handle table or not */
     if (!HandleTable) return STATUS_INSUFFICIENT_RESOURCES;
-
-    /* If we got here then the table was created OK */
     return STATUS_SUCCESS;
 }
 
@@ -1778,9 +1944,20 @@
 NTAPI
 ObKillProcess(IN PEPROCESS Process)
 {
-    PHANDLE_TABLE HandleTable = Process->ObjectTable;
+    PHANDLE_TABLE HandleTable;
     OBP_CLOSE_HANDLE_CONTEXT Context;
+    BOOLEAN HardErrors;
     PAGED_CODE();
+
+    /* Wait for process rundown */
+    ExWaitForRundownProtectionRelease(&Process->RundownProtect);
+
+    /* Get the object table */
+    HandleTable = Process->ObjectTable;
+    if (!HandleTable) return;
+
+    /* Disable hard errors while we close handles */
+    HardErrors = IoSetThreadHardErrorMode(FALSE);
 
     /* Enter a critical region */
     KeEnterCriticalRegion();
@@ -1793,13 +1970,20 @@
     ExSweepHandleTable(HandleTable,
                        ObpCloseHandleCallback,
                        &Context);
-
-    /* Destroy the table and leave the critical region */
+    if (HandleTable->HandleCount != 0)
+    {
+        DPRINT1("FIXME: %d handles remain!\n", HandleTable->HandleCount);
+    }
+
+    /* Leave the critical region */
+    KeLeaveCriticalRegion();
+
+    /* Re-enable hard errors */
+    IoSetThreadHardErrorMode(HardErrors);
+
+    /* Destroy the object table */
+    Process->ObjectTable = NULL;
     ExDestroyHandleTable(HandleTable);
-    KeLeaveCriticalRegion();
-
-    /* Clear the object table */
-    Process->ObjectTable = NULL;
 }
 
 NTSTATUS
@@ -1820,12 +2004,12 @@
     POBJECT_TYPE ObjectType;
     HANDLE NewHandle;
     KAPC_STATE ApcState;
-    NTSTATUS Status = STATUS_SUCCESS;
+    NTSTATUS Status;
     ACCESS_MASK TargetAccess, SourceAccess;
     ACCESS_STATE AccessState;
     PACCESS_STATE PassedAccessState = NULL;
     AUX_DATA AuxData;
-    PHANDLE_TABLE HandleTable = NULL;
+    PHANDLE_TABLE HandleTable;
     OBJECT_HANDLE_INFORMATION HandleInformation;
     PAGED_CODE();
     OBTRACE(OB_HANDLE_DEBUG,
@@ -1835,32 +2019,77 @@
             SourceProcess,
             TargetProcess);
 
-    /* Check if we're not in the source process */
-    if (SourceProcess != PsGetCurrentProcess())
-    {
-        /* Attach to it */
-        KeStackAttachProcess(&SourceProcess->Pcb, &ApcState);
-        AttachedToProcess = TRUE;
-    }
-
-    /* Now reference the source handle */
-    Status = ObReferenceObjectByHandle(SourceHandle,
-                                       0,
-                                       NULL,
-                                       PreviousMode,
-                                       (PVOID*)&SourceObject,
-                                       &HandleInformation);
-
-    /* Check if we were attached */
-    if (AttachedToProcess)
-    {
-        /* We can safely detach now */
-        KeUnstackDetachProcess(&ApcState);
-        AttachedToProcess = FALSE;
-    }
-
-    /* Fail if we couldn't reference it */
-    if (!NT_SUCCESS(Status)) return Status;
+    /* Check if we're not duplicating the same access */
+    if (!(Options & DUPLICATE_SAME_ACCESS))
+    {
+        /* Validate the desired access */
+        Status = STATUS_SUCCESS; //ObpValidateDesiredAccess(DesiredAccess);
+        if (!NT_SUCCESS(Status)) return Status;
+    }
+
+    /* Reference the object table */
+    HandleTable = ObReferenceProcessHandleTable(SourceProcess);
+    if (!HandleTable) return STATUS_PROCESS_IS_TERMINATING;
+
+    /* Reference the process object */
+    Status = ObpReferenceProcessObjectByHandle(SourceHandle,
+                                               0,
+                                               HandleTable,
+                                               PreviousMode,
+                                               &SourceObject,
+                                               &HandleInformation);
+    if (!NT_SUCCESS(Status))
+    {
+        /* Fail */
+        ObDereferenceProcessHandleTable(SourceProcess);
+        return Status;
+    }
+
+    /* Check if there's no target process */
+    if (!TargetProcess)
+    {
+        /* Check if the caller wanted actual duplication */
+        if (!(Options & DUPLICATE_CLOSE_SOURCE))
+        {
+            /* Invalid request */
+            Status = STATUS_INVALID_PARAMETER;
+        }
+        else
+        {
+            /* Otherwise, do the attach */
+            KeStackAttachProcess(&SourceProcess->Pcb, &ApcState);
+
+            /* Close the handle and detach */
+            NtClose(SourceHandle);
+            KeUnstackDetachProcess(&ApcState);
+        }
+
+        /* Return */
+        ObDereferenceProcessHandleTable(SourceProcess);
+        ObDereferenceObject(SourceObject);
+        return Status;
+    }
+
+    /* Get the target handle table */
+    HandleTable = ObReferenceProcessHandleTable(TargetProcess);
+    if (!HandleTable)
+    {
+        /* Check if the caller wanted us to close the handle */
+        if (Options & DUPLICATE_CLOSE_SOURCE)
+        {
+            /* Do the attach */
+            KeStackAttachProcess(&SourceProcess->Pcb, &ApcState);
+
+            /* Close the handle and detach */
+            NtClose(SourceHandle);
+            KeUnstackDetachProcess(&ApcState);
+        }
+
+        /* Return */
+        ObDereferenceProcessHandleTable(SourceProcess);
+        ObDereferenceObject(SourceObject);
+        return STATUS_PROCESS_IS_TERMINATING;
+    }
 
     /* Get the source access */
     SourceAccess = HandleInformation.GrantedAccess;
@@ -1898,7 +2127,8 @@
     if (DesiredAccess & GENERIC_ACCESS)
     {
         /* Map it */
-        RtlMapGenericMask(&DesiredAccess,
&ObjectType->TypeInfo.GenericMapping);
+        RtlMapGenericMask(&DesiredAccess,
+                          &ObjectType->TypeInfo.GenericMapping);
     }
 
     /* Set the target access */
@@ -1940,9 +2170,6 @@
                                          HandleAttributes,
                                          PsGetCurrentProcess(),
                                          ObDuplicateHandle);
-
-        /* Set the handle table, now that we know this handle was added */
-        HandleTable = PsGetCurrentProcess()->ObjectTable;
     }
 
     /* Check if we were attached */
@@ -1989,6 +2216,10 @@
 
     /* Return the handle */
     if (TargetHandle) *TargetHandle = NewHandle;
+
+    /* Dereference handle tables */
+    ObDereferenceProcessHandleTable(SourceProcess);
+    ObDereferenceProcessHandleTable(TargetProcess);
 
     /* Return status */
     OBTRACE(OB_HANDLE_DEBUG,

Modified: trunk/reactos/ntoskrnl/ob/obref.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ob/obref.c?rev=25…
==============================================================================
--- trunk/reactos/ntoskrnl/ob/obref.c (original)
+++ trunk/reactos/ntoskrnl/ob/obref.c Wed Jan 10 06:35:59 2007
@@ -478,8 +478,8 @@
     NTSTATUS Status;
     PAGED_CODE();
 
-    /* Fail immediately if the handle is NULL */
-    if (!Handle) return STATUS_INVALID_HANDLE;
+    /* Assume failure */
+    *Object = NULL;
 
     /* Check if the caller wants the current process */
     if ((Handle == NtCurrentProcess()) &&
@@ -488,9 +488,6 @@
         /* Get the current process */
         CurrentProcess = PsGetCurrentProcess();
 
-        /* Reference ourselves */
-        ObReferenceObject(CurrentProcess);
-
         /* Check if the caller wanted handle information */
         if (HandleInformation)
         {
@@ -499,6 +496,10 @@
             HandleInformation->GrantedAccess = PROCESS_ALL_ACCESS;
         }
 
+        /* Reference ourselves */
+        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentProcess);
+        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+
         /* Return the pointer */
         *Object = CurrentProcess;
         return STATUS_SUCCESS;
@@ -516,9 +517,6 @@
         /* Get the current thread */
         CurrentThread = PsGetCurrentThread();
 
-        /* Reference ourselves */
-        ObReferenceObject(CurrentThread);
-
         /* Check if the caller wanted handle information */
         if (HandleInformation)
         {
@@ -527,6 +525,10 @@
             HandleInformation->GrantedAccess = THREAD_ALL_ACCESS;
         }
 
+        /* Reference ourselves */
+        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentThread);
+        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+
         /* Return the pointer */
         *Object = CurrentThread;
         return STATUS_SUCCESS;
@@ -551,6 +553,7 @@
     }
 
     /* Enter a critical region while we touch the handle table */
+    ASSERT(HandleTable != NULL);
     KeEnterCriticalRegion();
 
     /* Get the handle entry */
@@ -588,9 +591,10 @@
 
                 /* Unlock the handle */
                 ExUnlockHandleTableEntry(HandleTable, HandleEntry);
+                KeLeaveCriticalRegion();
 
                 /* Return success */
-                KeLeaveCriticalRegion();
+                ASSERT(*Object != NULL);
                 return STATUS_SUCCESS;
             }
             else