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