win2k poroblem

[Hartmut Birr]

Hi,

while looking for the console closing problem, I've seen that win2k
calls PsLookupProcessByProcessId very often with a id of 0xffffffff.

- Hartmut


(ex/handle.c:721) Looking up invalid handle 0xffffffff
Frames:
<ntoskrnl.exe:26f2d (ex/handle.c:722 (ExpLookupHandleTableEntry))>
<ntoskrnl.exe:275ce (ex/handle.c:919 (ExMapHandleToPointer))>
<ntoskrnl.exe:74af8 (ps/cid.c:106 (PsLookupCidHandle))>
<ntoskrnl.exe:7c6d5 (ps/process.c:2709 (PsLookupProcessByProcessId))>
<win32k.sys:45c16 (objects/gdiobj.c:1219 (GDIOBJ_SetOwnership))>
<win32k.sys:6840 (eng/surface.c:466 (EngDeleteSurface))>
<win32k.sys:52456 (objects/text.c:1922 (NtGdiExtTextOut))>
<win32k.sys:539ef (objects/text.c:2770 (NtGdiTextOut))>
<ntoskrnl.exe:3fb2 (D:\DOKUME~1\hb\LOKALE~1\Temp/ccgPaaaa.s:178
(KiSystemService))>
<gdi32.dll:99bc (objects/text.c:45 (TextOutW))>

[Mike Nordell]

Hartmut Birr wrote:

>   while looking for the console closing problem,
> I've seen that win2k
>  calls PsLookupProcessByProcessId very often with a id of 0xffffffff. 
~0 (or -1) is in Win32 (and therefore by extension likely also for NT) the
"pseudo handle" for the current process.

~1 (or -2) is current thread.


/Mike

[Hartmut Birr]

Mike Nordell schrieb:

>  Hartmut Birr wrote:
>
>   
>
> >  while looking for the console closing problem,
> > I've seen that win2k
> > calls PsLookupProcessByProcessId very often with a id of 0xffffffff.
> >     
> >  
> ~0 (or -1) is in Win32 (and therefore by extension likely also for NT) the
> "pseudo handle" for the current process.
>
> ~1 (or -2) is current thread.
>   
>  
I know this. The value -1 is not used as pseudo handle. The value is
passed as id to PsLookupProcessByProcessId and that is wrong.

- Hartmut

[Filip Navara]

Hartmut Birr wrote:

>  Hi,
>
> while looking for the console closing problem, I've seen that win2k
> calls PsLookupProcessByProcessId very often with a id of 0xffffffff.
>
> - Hartmut
>   
>  
I've put ASSERT(ProcessId != (HANDLE)-1) in PsLookupProcessByProcessId 
on my local copy and it was never triggered. My test was booting to 
Explorer and running OpenOffice.org 1.1.1 Word Processor...

>  (ex/handle.c:721) Looking up invalid handle 0xffffffff
> Frames:
> <ntoskrnl.exe:26f2d (ex/handle.c:722 (ExpLookupHandleTableEntry))>
> <ntoskrnl.exe:275ce (ex/handle.c:919 (ExMapHandleToPointer))>
> <ntoskrnl.exe:74af8 (ps/cid.c:106 (PsLookupCidHandle))>
> <ntoskrnl.exe:7c6d5 (ps/process.c:2709 (PsLookupProcessByProcessId))>
> <win32k.sys:45c16 (objects/gdiobj.c:1219 (GDIOBJ_SetOwnership))>
>   
>  
^ Honestly I can't see how can you ever get ProcessId == -1 from this 
line of code (assuming it's the correct line):
    Status = PsLookupProcessByProcessId((HANDLE)((ULONG_PTR)PrevProcId & 
~0x1), &OldProcess);
Since a "binary and" with 0xfffffffe is performed, the result can never 
be 0xfffffffff.

>  <win32k.sys:6840 (eng/surface.c:466
> (EngDeleteSurface))>
> <win32k.sys:52456 (objects/text.c:1922 (NtGdiExtTextOut))>
> <win32k.sys:539ef (objects/text.c:2770 (NtGdiTextOut))>
> <ntoskrnl.exe:3fb2 (D:\DOKUME~1\hb\LOKALE~1\Temp/ccgPaaaa.s:178
> (KiSystemService))>
> <gdi32.dll:99bc (objects/text.c:45 (TextOutW))>
>   
>  
Regards,
Filip

[Hartmut Birr]

Hi,

I get this again. I'm using cmd as login shell and starting the
explorer. I get hundreds of lines like this:

(ex/handle.c:721) Looking up invalid handle 0xffffffff
Frames: <ntoskrnl.exe:26efd (ex/handle.c:722 (ExpLookupHandleTableEntry))>
<ntoskrnl.exe:27576 (ex/handle.c:915 (ExMapHandleToPointer))>
<ntoskrnl.exe:748d8 (ps/cid.c:106 (PsLookupCidHandle))>
<ntoskrnl.exe:7c497 (ps/process.c:2709 (PsLookupProcessByProcessId))>
<win32k.sys:464d6 (objects/gdiobj.c:1219 (GDIOBJ_SetOwnership))>
<win32k.sys:6840 (eng/surface.c:466 (EngDeleteSurface))>
 <win32k.sys:52d16 (objects/text.c:1922 (NtGdiExtTextOut))>
<ntoskrnl.exe:3fb2 (D:\DOKUME~1\hb\LOKALE~1\Temp/ccUVaaaa.s:178
(KiSystemService))>
<gdi32.dll:9c22 (objects/text.c:272 (ExtTextOutW))>

The starting point does change but GDIOBJ_SetOwnership and later are
always the same. I attach my changes in ntoskrnl. I think that the
changes in ob/handle.c are not relevant. It is the smp build on my smp
machine.

- Hartmut

Filip Navara schrieb:

>   Hartmut Birr wrote:
>
> >   Hi,
> >
> >  while looking for the console closing problem, I've seen that win2k
> >  calls PsLookupProcessByProcessId very often with a id of 0xffffffff.
> >
> >  - Hartmut
> >   
> >  
>  I've put ASSERT(ProcessId != (HANDLE)-1) in PsLookupProcessByProcessId
>  on my local copy and it was never triggered. My test was booting to
>  Explorer and running OpenOffice.org 1.1.1 Word Processor...
>
> >   (ex/handle.c:721) Looking up invalid handle
> > 0xffffffff
> >  Frames:
> >  <ntoskrnl.exe:26f2d (ex/handle.c:722 (ExpLookupHandleTableEntry))>
> >  <ntoskrnl.exe:275ce (ex/handle.c:919 (ExMapHandleToPointer))>
> >  <ntoskrnl.exe:74af8 (ps/cid.c:106 (PsLookupCidHandle))>
> >  <ntoskrnl.exe:7c6d5 (ps/process.c:2709 (PsLookupProcessByProcessId))>
> >  <win32k.sys:45c16 (objects/gdiobj.c:1219 (GDIOBJ_SetOwnership))>
> >   
> >  
>  ^ Honestly I can't see how can you ever get ProcessId == -1 from this
>  line of code (assuming it's the correct line):
>     Status = PsLookupProcessByProcessId((HANDLE)((ULONG_PTR)PrevProcId
>  & ~0x1), &OldProcess);
>  Since a "binary and" with 0xfffffffe is performed, the result can
>  never be 0xfffffffff.
>
> >   <win32k.sys:6840 (eng/surface.c:466
> > (EngDeleteSurface))>
> >  <win32k.sys:52456 (objects/text.c:1922 (NtGdiExtTextOut))>
> >  <win32k.sys:539ef (objects/text.c:2770 (NtGdiTextOut))>
> >  <ntoskrnl.exe:3fb2 (D:\DOKUME~1\hb\LOKALE~1\Temp/ccgPaaaa.s:178
> >  (KiSystemService))>
> >  <gdi32.dll:99bc (objects/text.c:45 (TextOutW))>
> >   
> >  
>  Regards,
>  Filip
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.com
>  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\handle.c ntoskrnl\ex\handle.c       
Index: ntoskrnl/ob/handle.c
===================================================================
--- ntoskrnl/ob/handle.c	(revision 14161)
+++ ntoskrnl/ob/handle.c	(working copy)
@@ -160,6 +160,7 @@
   POBJECT_HEADER ObjectHeader;
   LONG ExTargetHandle;
   LONG ExSourceHandle = HANDLE_TO_EX_HANDLE(SourceHandle);
+  ULONG NewHandleCount;
   
   PAGED_CODE();
   
@@ -194,8 +195,8 @@
      1 here, we're in big trouble... it would've been safe to increment and
      check the handle count without using interlocked functions because the
      entry is locked, which means the handle count can't change. */
-  InterlockedIncrement(&ObjectHeader->HandleCount);
-  ASSERT(ObjectHeader->HandleCount >= 2);
+  NewHandleCount = InterlockedIncrement(&ObjectHeader->HandleCount);
+  ASSERT(NewHandleCount >= 2);
   
   ExUnlockHandleTableEntry(SourceProcess->ObjectTable,
                            SourceHandleEntry);
@@ -323,7 +324,8 @@
      }
 
    /* Check for magic handle first */
-   if (SourceHandle == NtCurrentThread())
+   if (SourceHandle == NtCurrentThread() ||
+       SourceHandle == NtCurrentProcess())
      {
        PVOID ObjectBody;
        
@@ -425,10 +427,7 @@
     ObjectHeader = EX_HTE_TO_HDR(HandleTableEntry);
     if(InterlockedIncrement(&ObjectHeader->HandleCount) == 1)
     {
-      ObReferenceObjectByPointer(HEADER_TO_BODY(ObjectHeader),
-			         0,
-			         NULL,
-			         UserMode);
+      ObReferenceObject(HEADER_TO_BODY(ObjectHeader));
     }
   }
   
@@ -555,10 +554,7 @@
    {
      if(InterlockedIncrement(&ObjectHeader->HandleCount) == 1)
      {
-      ObReferenceObjectByPointer(ObjectBody,
-			         0,
-			         NULL,
-			         UserMode);
+       ObReferenceObject(ObjectBody);
      }
      
      *HandleReturn = EX_HANDLE_TO_HANDLE(ExHandle);
@@ -740,6 +736,7 @@
                                  HandleEntry);
 
         KeLeaveCriticalRegion();
+        ObDereferenceObject(ObjectBody);
         
         return(STATUS_OBJECT_TYPE_MISMATCH);
      }
@@ -756,6 +753,7 @@
 	if (!(GrantedAccess & DesiredAccess) &&
 	    !((~GrantedAccess) & DesiredAccess))
 	  {
+             ObDereferenceObject(ObjectBody);
 	     CHECKPOINT;
 	     return(STATUS_ACCESS_DENIED);
 	  }
Index: ntoskrnl/ex/handle.c
===================================================================
--- ntoskrnl/ex/handle.c	(revision 14161)
+++ ntoskrnl/ex/handle.c	(working copy)
@@ -718,7 +718,9 @@
   }
   else
   {
-    DPRINT("Looking up invalid handle 0x%x\n", Handle);
+    DPRINT1("Looking up invalid handle 0x%x\n", Handle);
+    KeRosDumpStackFrames(NULL, 15);
+
   }
   
   return Entry;

[Filip Navara]

Hartmut Birr wrote:

>  Hi,
>
> I get this again. I'm using cmd as login shell and starting the
> explorer. I get hundreds of lines like this:
>
> (ex/handle.c:721) Looking up invalid handle 0xffffffff
> Frames: <ntoskrnl.exe:26efd (ex/handle.c:722 (ExpLookupHandleTableEntry))>
> <ntoskrnl.exe:27576 (ex/handle.c:915 (ExMapHandleToPointer))>
> <ntoskrnl.exe:748d8 (ps/cid.c:106 (PsLookupCidHandle))>
> <ntoskrnl.exe:7c497 (ps/process.c:2709 (PsLookupProcessByProcessId))>
> <win32k.sys:464d6 (objects/gdiobj.c:1219 (GDIOBJ_SetOwnership))>
> <win32k.sys:6840 (eng/surface.c:466 (EngDeleteSurface))>
>  <win32k.sys:52d16 (objects/text.c:1922 (NtGdiExtTextOut))>
> <ntoskrnl.exe:3fb2 (D:\DOKUME~1\hb\LOKALE~1\Temp/ccUVaaaa.s:178
> (KiSystemService))>
> <gdi32.dll:9c22 (objects/text.c:272 (ExtTextOutW))>
>
> The starting point does change but GDIOBJ_SetOwnership and later are
> always the same. I attach my changes in ntoskrnl. I think that the
> changes in ob/handle.c are not relevant. It is the smp build on my smp
> machine.
>   
>  
Does the attached patch help? I noticed that the handle number gets 
converted along the way by the HANDLE_TO_EX_HANDLE macro and in fact the 
original ID passed to PsLookupProcessByProcessId is zero. This makes 
prefect sense since it's used as a marker for global GDI handles.

- Filip

P.S. Sorry for the ugly grammar mistakes in my previous post. I should 
read twice what I wrote before hitting the Send button. ;-)

[Hartmut Birr]

Filip Navara schrieb:

>   Does the attached patch help? I noticed that the
> handle number gets
>  converted along the way by the HANDLE_TO_EX_HANDLE macro and in fact
>  the original ID passed to PsLookupProcessByProcessId is zero. This
>  makes prefect sense since it's used as a marker for global GDI handles. 
Your patch fix the problem. Now I see only ten or twenty calls to
ExpLookupHandleTableEntry with -1 or -2 from other locations. Most of
them are from CloseHandle and some from ReleaseMutex.

>  
>  - Filip
>
>  P.S. Sorry for the ugly grammar mistakes in my previous post. I should
>  read twice what I wrote before hitting the Send button. ;-) 
And I did always mean win32k in the topic and not win2k.

- Hartmut