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
> [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\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