[hbirr] 14162: - Moved the deleting of the process id from PiTerminateProcess to PiDeleteProcessWorker.

- Moved the deleting of the process id from PiTerminateProcess to
PiDeleteProcessWorker.  
- Checked the status at the end of NtOpenProcessTokenEx.
Modified: trunk/reactos/ntoskrnl/ps/kill.c
Modified: trunk/reactos/ntoskrnl/ps/process.c
  _____

Modified: trunk/reactos/ntoskrnl/ps/kill.c
--- trunk/reactos/ntoskrnl/ps/kill.c	2005-03-17 16:09:12 UTC (rev
14161)
+++ trunk/reactos/ntoskrnl/ps/kill.c	2005-03-17 18:51:20 UTC (rev
14162)
@@ -329,11 +329,6 @@

}

ObDeleteHandleTable(Process);
-   if(Process->UniqueProcessId != NULL)
-   {
-     PsDeleteCidHandle(Process->UniqueProcessId, PsProcessType);
-   }
-
    if (Process != CurrentProcess)
    {
       KeDetachProcess();
  _____

Modified: trunk/reactos/ntoskrnl/ps/process.c
--- trunk/reactos/ntoskrnl/ps/process.c	2005-03-17 16:09:12 UTC (rev
14161)
+++ trunk/reactos/ntoskrnl/ps/process.c	2005-03-17 18:51:20 UTC (rev
14162)
@@ -243,15 +243,23 @@

&hToken);
      ObDereferenceObject(Token);

-     _SEH_TRY
+     if (NT_SUCCESS(Status))
      {
-       *TokenHandle = hToken;
+
+       _SEH_TRY
+       {
+         *TokenHandle = hToken;
+       }
+       _SEH_HANDLE
+       {
+         Status = _SEH_GetExceptionCode();
+       }
+       _SEH_END;
+       if (!NT_SUCCESS(Status))
+       {
+         NtClose(hToken);
+       }
      }
-     _SEH_HANDLE
-     {
-       Status = _SEH_GetExceptionCode();
-     }
-     _SEH_END;
    }

return Status;
@@ -518,6 +526,11 @@
       KeDetachProcess();
     }

+  if(Process->UniqueProcessId != NULL)
+    {
+      PsDeleteCidHandle(Process->UniqueProcessId, PsProcessType);
+    }
+
   MmReleaseMmInfo(Process);
   if (Context->IsWorkerQueue)
     {
@@ -1371,7 +1384,6 @@
    DPRINT("NtOpenProcess() = STATUS_UNSUCCESSFUL\n");
    return(STATUS_UNSUCCESSFUL);
      }
-   return(STATUS_UNSUCCESSFUL);
 }

[Thomas Weidenmueller]

[e-mail] wrote:
>   if (!NT_SUCCESS(Status))
>   {
>     NtClose(hToken);
>   }

This is not correct. If writing back the handle failed, it should not be 
closed. It actually is a very rare condition if called from user mode, 
but if called from kernel mode the handle pointer doesn't get probed, so 
if you call the function and pass it a bad pointer you'll notice that 
the process will have one additional handle in it's handle table, which 
actually is the handle created in that function. It's also possible to 
find that handles using ObFindHandleForObject (if there's only one 
handle of the requested object type). However, I don't know if windows 
returns an error code or success in case just writing back the handle 
failed, i might have to write a test case for this. But if they actually 
return success we've got a bunch of functions to fix...

Best Regards,
Thomas

[Gunnar Dalsnes]

> This is not correct. If writing back the handle failed, it should not be 
> closed. It actually is a very rare condition if called from user mode, 
> but if called from kernel mode the handle pointer doesn't get probed, so 
> if you call the function and pass it a bad pointer you'll notice that 
> the process will have one additional handle in it's handle table, which 
> actually is the handle created in that function. It's also possible to

Sounds more like a bug than a "feature". And if this was a feature, we 
would all be expected to try to find the possibly created handle in the 
handletable every time a handle creating routine failed:-)

G.

[Thomas Weidenmueller]

Gunnar Dalsnes wrote:
> Sounds more like a bug than a "feature". And if this was a feature, we 
> would all be expected to try to find the possibly created handle in 
> the handletable every time a handle creating routine failed:-)

No, it's not a bug. A function might have created directories, symbolic 
links, driver objects, ... that operated already, if you closed the 
handle you might destroy some of these things even though other 
applications or other parts of the operating system are already aware of 
them and all the sudden e.g. devices might vanish. So it actually makes 
sense. Remember, some objects perform certain operations (like closing 
completion ports) if the last handle was closed.

Best Regards,
Thomas