Author: ion
Date: Sat Jul 23 18:28:55 2011
New Revision: 52813
URL: http://svn.reactos.org/svn/reactos?rev=52813&view=rev
Log:
[KERNEL32]: User-mode Debugging fixes.
Bug: RemoveHandles was checking for process AND thread handle match, instead of OR.
Bug: CloseAllProcessHandles and RemoveHandles's processing loop was buggy with its unlinking and re-use of nested ThreadData.
Bug: ProcessIdToHandle was requesting more process rights than needed.
Bug: Some ULONG<->BOOL conversions were dirty (as in, could result in values of 2 or higher).
Bug: In WaitForDebugEvent, During CREATE_PROCESS_DEBUG_EVENT, the wrong handle was saved as the thread handle.
Bug: In WaitForDebugEvent, all events returned TRUE. Instead, only valid events should return TRUE, and non-existing events should return FALSE.
Add some asserts.
Simplify MarkThreadHandle and MarkProcessHandle.
Simplify OutputDebugStringW.
Modified:
trunk/reactos/dll/win32/kernel32/client/debugger.c
Modified: trunk/reactos/dll/win32/kernel32/client/debugger.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/debugger.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/debugger.c [iso-8859-1] Sat Jul 23 18:28:55 2011
@@ -264,8 +264,7 @@
PDBGSS_THREAD_DATA ThreadData;
/* Loop all thread data events */
- ThreadData = DbgSsGetThreadData();
- while (ThreadData)
+ for (ThreadData = DbgSsGetThreadData(); ThreadData; ThreadData = ThreadData->Next)
{
/* Check if this one matches */
if (ThreadData->ThreadId == dwThreadId)
@@ -274,9 +273,6 @@
ThreadData->HandleMarked = TRUE;
break;
}
-
- /* Move to the next one */
- ThreadData = ThreadData->Next;
}
}
@@ -285,116 +281,81 @@
MarkProcessHandle(IN DWORD dwProcessId)
{
PDBGSS_THREAD_DATA ThreadData;
+
+ /* Loop all thread data events */
+ for (ThreadData = DbgSsGetThreadData(); ThreadData; ThreadData = ThreadData->Next)
+ {
+ /* Check if this one matches */
+ if ((ThreadData->ProcessId == dwProcessId) && !(ThreadData->ThreadId))
+ {
+ /* Mark the structure and break out */
+ ThreadData->HandleMarked = TRUE;
+ break;
+ }
+ }
+}
+
+VOID
+WINAPI
+RemoveHandles(IN DWORD dwProcessId,
+ IN DWORD dwThreadId)
+{
+ PDBGSS_THREAD_DATA ThreadData, ThisData;
/* Loop all thread data events */
ThreadData = DbgSsGetThreadData();
- while (ThreadData)
+ for (ThisData = ThreadData; ThisData; ThisData = ThisData->Next)
{
/* Check if this one matches */
- if (ThreadData->ProcessId == dwProcessId)
+ if ((ThisData->HandleMarked) &&
+ ((ThisData->ProcessId == dwProcessId) || (ThisData->ThreadId == dwThreadId)))
{
- /* Make sure the thread ID is empty */
- if (!ThreadData->ThreadId)
- {
- /* Mark the structure and break out */
- ThreadData->HandleMarked = TRUE;
- break;
- }
+ /* Close open handles */
+ if (ThisData->ThreadHandle) CloseHandle(ThisData->ThreadHandle);
+ if (ThisData->ProcessHandle) CloseHandle(ThisData->ProcessHandle);
+
+ /* Unlink the thread data */
+ ThreadData->Next = ThisData->Next;
+
+ /* Free it*/
+ RtlFreeHeap(RtlGetProcessHeap(), 0, ThisData);
}
-
- /* Move to the next one */
- ThreadData = ThreadData->Next;
+ else
+ {
+ /* Move to the next one */
+ ThreadData = ThisData;
+ }
}
}
VOID
WINAPI
-RemoveHandles(IN DWORD dwProcessId,
- IN DWORD dwThreadId)
-{
- PDBGSS_THREAD_DATA ThreadData;
+CloseAllProcessHandles(IN DWORD dwProcessId)
+{
+ PDBGSS_THREAD_DATA ThreadData, ThisData;
/* Loop all thread data events */
ThreadData = DbgSsGetThreadData();
- while (ThreadData)
+ for (ThisData = ThreadData; ThisData; ThisData = ThisData->Next)
{
/* Check if this one matches */
- if (ThreadData->ProcessId == dwProcessId)
+ if (ThisData->ProcessId == dwProcessId)
{
- /* Make sure the thread ID matches too */
- if (ThreadData->ThreadId == dwThreadId)
- {
- /* Check if we have a thread handle */
- if (ThreadData->ThreadHandle)
- {
- /* Close it */
- CloseHandle(ThreadData->ThreadHandle);
- }
-
- /* Check if we have a process handle */
- if (ThreadData->ProcessHandle)
- {
- /* Close it */
- CloseHandle(ThreadData->ProcessHandle);
- }
-
- /* Unlink the thread data */
- DbgSsSetThreadData(ThreadData->Next);
-
- /* Free it*/
- RtlFreeHeap(RtlGetProcessHeap(), 0, ThreadData);
-
- /* Move to the next structure */
- ThreadData = DbgSsGetThreadData();
- continue;
- }
+ /* Close open handles */
+ if (ThisData->ThreadHandle) CloseHandle(ThisData->ThreadHandle);
+ if (ThisData->ProcessHandle) CloseHandle(ThisData->ProcessHandle);
+
+ /* Unlink the thread data */
+ ThreadData->Next = ThisData->Next;
+
+ /* Free it*/
+ RtlFreeHeap(RtlGetProcessHeap(), 0, ThisData);
}
-
- /* Move to the next one */
- ThreadData = ThreadData->Next;
- }
-}
-
-VOID
-WINAPI
-CloseAllProcessHandles(IN DWORD dwProcessId)
-{
- PDBGSS_THREAD_DATA ThreadData;
-
- /* Loop all thread data events */
- ThreadData = DbgSsGetThreadData();
- while (ThreadData)
- {
- /* Check if this one matches */
- if (ThreadData->ProcessId == dwProcessId)
+ else
{
- /* Check if we have a thread handle */
- if (ThreadData->ThreadHandle)
- {
- /* Close it */
- CloseHandle(ThreadData->ThreadHandle);
- }
-
- /* Check if we have a process handle */
- if (ThreadData->ProcessHandle)
- {
- /* Close it */
- CloseHandle(ThreadData->ProcessHandle);
- }
-
- /* Unlink the thread data */
- DbgSsSetThreadData(ThreadData->Next);
-
- /* Free it*/
- RtlFreeHeap(RtlGetProcessHeap(), 0, ThreadData);
-
- /* Move to the next structure */
- ThreadData = DbgSsGetThreadData();
- continue;
+ /* Move to the next one */
+ ThreadData = ThisData;
}
-
- /* Move to the next one */
- ThreadData = ThreadData->Next;
}
}
@@ -415,7 +376,9 @@
ClientId.UniqueProcess = UlongToHandle(dwProcessId);
InitializeObjectAttributes(&ObjectAttributes, NULL, 0, NULL, NULL);
Status = NtOpenProcess(&Handle,
- PROCESS_ALL_ACCESS,
+ PROCESS_CREATE_THREAD | PROCESS_VM_OPERATION |
+ PROCESS_VM_WRITE | PROCESS_VM_READ |
+ PROCESS_SUSPEND_RESUME | PROCESS_QUERY_INFORMATION,
&ObjectAttributes,
&ClientId);
if (!NT_SUCCESS(Status))
@@ -459,7 +422,7 @@
if (NT_SUCCESS(Status))
{
/* Return the current state */
- *pbDebuggerPresent = (DebugPort) ? TRUE : FALSE;
+ *pbDebuggerPresent = DebugPort != NULL;
return TRUE;
}
@@ -507,7 +470,7 @@
WINAPI
DebugActiveProcess(IN DWORD dwProcessId)
{
- NTSTATUS Status;
+ NTSTATUS Status, Status1;
HANDLE Handle;
/* Connect to the debugger */
@@ -524,7 +487,10 @@
/* Now debug the process */
Status = DbgUiDebugActiveProcess(Handle);
- NtClose(Handle);
+
+ /* Close the handle since we're done */
+ Status1 = NtClose(Handle);
+ ASSERT(NT_SUCCESS(Status1));
/* Check if debugging worked */
if (!NT_SUCCESS(Status))
@@ -545,7 +511,7 @@
WINAPI
DebugActiveProcessStop(IN DWORD dwProcessId)
{
- NTSTATUS Status;
+ NTSTATUS Status, Status1;
HANDLE Handle;
/* Get the process handle */
@@ -557,7 +523,8 @@
/* Now stop debgging the process */
Status = DbgUiStopDebugging(Handle);
- NtClose(Handle);
+ Status1 = NtClose(Handle);
+ ASSERT(NT_SUCCESS(Status1));
/* Check for failure */
if (!NT_SUCCESS(Status))
@@ -614,7 +581,7 @@
}
/* Now set the kill-on-exit state */
- State = KillOnExit;
+ State = KillOnExit != 0;
Status = NtSetInformationDebugObject(Handle,
DebugObjectKillProcessOnExitInformation,
&State,
@@ -711,15 +678,14 @@
/* Setup the thread data */
SaveThreadHandle(lpDebugEvent->dwProcessId,
lpDebugEvent->dwThreadId,
- lpDebugEvent->u.CreateThread.hThread);
+ lpDebugEvent->u.CreateProcessInfo.hThread);
break;
/* Process was exited */
case EXIT_PROCESS_DEBUG_EVENT:
- /* Mark the thread data as such */
+ /* Mark the thread data as such and fall through */
MarkProcessHandle(lpDebugEvent->dwProcessId);
- break;
/* Thread was exited */
case EXIT_THREAD_DEBUG_EVENT:
@@ -727,10 +693,18 @@
/* Mark the thread data */
MarkThreadHandle(lpDebugEvent->dwThreadId);
break;
-
- /* Nothing to do for anything else */
+
+ /* Nothing to do */
+ case EXCEPTION_DEBUG_EVENT:
+ case LOAD_DLL_DEBUG_EVENT:
+ case UNLOAD_DLL_DEBUG_EVENT:
+ case OUTPUT_DEBUG_STRING_EVENT:
+ case RIP_EVENT:
+ break;
+
+ /* Fail anything else */
default:
- break;
+ return FALSE;
}
/* Return success */
@@ -957,29 +931,24 @@
*/
VOID
WINAPI
-OutputDebugStringW(IN LPCWSTR _OutputString)
-{
- UNICODE_STRING wstrOut;
- ANSI_STRING strOut;
- NTSTATUS nErrCode;
+OutputDebugStringW(IN LPCWSTR OutputString)
+{
+ UNICODE_STRING UnicodeString;
+ ANSI_STRING AnsiString;
+ NTSTATUS Status;
/* convert the string in ANSI */
- RtlInitUnicodeString(&wstrOut, _OutputString);
- nErrCode = RtlUnicodeStringToAnsiString(&strOut, &wstrOut, TRUE);
-
- if(!NT_SUCCESS(nErrCode))
- {
- /* Microsoft's kernel32.dll always prints something, even in case the conversion fails */
- OutputDebugStringA("");
- }
- else
- {
- /* output the converted string */
- OutputDebugStringA(strOut.Buffer);
-
- /* free the converted string */
- RtlFreeAnsiString(&strOut);
- }
+ RtlInitUnicodeString(&UnicodeString, OutputString);
+ Status = RtlUnicodeStringToAnsiString(&AnsiString, &UnicodeString, TRUE);
+
+ /* OutputDebugStringW always prints something, even if conversion fails */
+ if (!NT_SUCCESS(Status)) AnsiString.Buffer = "";
+
+ /* Output the converted string */
+ OutputDebugStringA(AnsiString.Buffer);
+
+ /* free the converted string */
+ if (NT_SUCCESS(Status)) RtlFreeAnsiString(&AnsiString);
}
/* EOF */
Author: ion
Date: Sat Jul 23 16:26:03 2011
New Revision: 52809
URL: http://svn.reactos.org/svn/reactos?rev=52809&view=rev
Log:
Thanks to Timo Kreuzer for discovering what led to these:
[KERNEL32]: BasepInitializeContext was not creating a correct CONTEXT record for fibers: the stack return address was not set (EIP was being used instead), and support for FPU-compatible Fibers was non-existent.
[KERNEL32]: CreateFiberEx was not passing the correct context flags to BasepInitializeContext to notify it that this is an FPU-fiber.
[KERNEL32]: SwitchToFiber was using some weird "FXSR" constant that maps to checking of PowerPC 64-bit Move instructions are available. We actually want to check for XMMI.
Modified:
trunk/reactos/dll/win32/kernel32/client/fiber.c
trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
trunk/reactos/dll/win32/kernel32/client/utils.c
Modified: trunk/reactos/dll/win32/kernel32/client/fiber.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/fiber.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/fiber.c [iso-8859-1] Sat Jul 23 16:26:03 2011
@@ -223,11 +223,8 @@
Fiber->ActivationContextStack = ActivationContextStack;
Fiber->Context.ContextFlags = CONTEXT_FULL;
- /* Save FPU State if requsted */
- if (dwFlags & FIBER_FLAG_FLOAT_SWITCH)
- {
- Fiber->Context.ContextFlags |= CONTEXT_FLOATING_POINT;
- }
+ /* Save FPU State if requested */
+ Fiber->Context.ContextFlags = (dwFlags & FIBER_FLAG_FLOAT_SWITCH) ? CONTEXT_FLOATING_POINT : 0;
/* initialize the context for the fiber */
BasepInitializeContext(&Fiber->Context,
Modified: trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/i386/fiber.S [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/i386/fiber.S [iso-8859-1] Sat Jul 23 16:26:03 2011
@@ -35,7 +35,7 @@
fnstcw [eax+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]
/* Check if the CPU supports SIMD MXCSR State Save */
- cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 1
+ cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
jnz NoFpuStateSave
stmxcsr [eax+FIBER_CONTEXT_DR6]
@@ -99,7 +99,7 @@
ControlWordEqual:
/* Load the new one */
- cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 1
+ cmp byte ptr ds:[PF_XMMI_INSTRUCTIONS_AVAILABLE], 1
jnz NoFpuStateRestore
ldmxcsr [ecx+FIBER_CONTEXT_DR6]
Modified: trunk/reactos/dll/win32/kernel32/client/utils.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/utils.c [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/utils.c [iso-8859-1] Sat Jul 23 16:26:03 2011
@@ -336,8 +336,9 @@
IN ULONG ContextType)
{
#ifdef _M_IX86
+ ULONG ContextFlags;
DPRINT("BasepInitializeContext: %p\n", Context);
-
+
/* Setup the Initial Win32 Thread Context */
Context->Eax = (ULONG)StartAddress;
Context->Ebx = (ULONG)Parameter;
@@ -352,30 +353,53 @@
Context->SegSs = KGDT_R3_DATA | RPL_MASK;
Context->SegGs = 0;
+ /* Set the Context Flags */
+ ContextFlags = Context->ContextFlags;
+ Context->ContextFlags = CONTEXT_FULL;
+
+ /* Give it some room for the Parameter */
+ Context->Esp -= sizeof(PVOID);
+
/* Set the EFLAGS */
Context->EFlags = 0x3000; /* IOPL 3 */
- if (ContextType == 1) /* For Threads */
- {
+ /* What kind of context is being created? */
+ if (ContextType == 1)
+ {
+ /* For Threads */
Context->Eip = (ULONG)BaseThreadStartupThunk;
}
- else if (ContextType == 2) /* For Fibers */
- {
- Context->Eip = (ULONG)BaseFiberStartup;
- }
- else /* For first thread in a Process */
- {
+ else if (ContextType == 2)
+ {
+ /* This is a fiber: make space for the return address */
+ Context->Esp -= sizeof(PVOID);
+ *((PVOID*)Context->Esp) = BaseFiberStartup;
+
+ /* Is FPU state required? */
+ Context->ContextFlags |= ContextFlags;
+ if (ContextFlags == CONTEXT_FLOATING_POINT)
+ {
+ /* Set an initial state */
+ Context->FloatSave.ControlWord = 0x27F;
+ Context->FloatSave.StatusWord = 0;
+ Context->FloatSave.TagWord = 0xFFFF;
+ Context->FloatSave.ErrorOffset = 0;
+ Context->FloatSave.ErrorSelector = 0;
+ Context->FloatSave.DataOffset = 0;
+ Context->FloatSave.DataSelector = 0;
+ if (SharedUserData->ProcessorFeatures[PF_XMMI_INSTRUCTIONS_AVAILABLE])
+ Context->Dr6 = 0x1F80;
+ }
+ }
+ else
+ {
+ /* For first thread in a Process */
Context->Eip = (ULONG)BaseProcessStartThunk;
}
-
- /* Set the Context Flags */
- Context->ContextFlags = CONTEXT_FULL;
-
- /* Give it some room for the Parameter */
- Context->Esp -= sizeof(PVOID);
+
#elif defined(_M_AMD64)
DPRINT("BasepInitializeContext: %p\n", Context);
-
+
/* Setup the Initial Win32 Thread Context */
Context->Rax = (ULONG_PTR)StartAddress;
Context->Rbx = (ULONG_PTR)Parameter;
@@ -405,10 +429,10 @@
{
Context->Rip = (ULONG_PTR)BaseProcessStartThunk;
}
-
+
/* Set the Context Flags */
Context->ContextFlags = CONTEXT_FULL;
-
+
/* Give it some room for the Parameter */
Context->Rsp -= sizeof(PVOID);
#else
Author: ion
Date: Sat Jul 23 12:08:36 2011
New Revision: 52807
URL: http://svn.reactos.org/svn/reactos?rev=52807&view=rev
Log:
[KERNEL32]: Optimize SwitchToFiber to simply use "ret" to jump between fibers, instead of saving EIP and doing a JMP.
Bug #50: SwitchToFiber needs to check if FXSR is *NOT* present in order to skip using ldmxcsr/stmxcsr. Previously, it would check if it's unsupported, and jump past the instruction if it was (resulting in invalid opcode instructions on older systems)
50 bugs. Penance has been paid.
Modified:
trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
Modified: trunk/reactos/dll/win32/kernel32/client/i386/fiber.S
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/dll/win32/kernel32/client/…
==============================================================================
--- trunk/reactos/dll/win32/kernel32/client/i386/fiber.S [iso-8859-1] (original)
+++ trunk/reactos/dll/win32/kernel32/client/i386/fiber.S [iso-8859-1] Sat Jul 23 12:08:36 2011
@@ -26,20 +26,16 @@
mov [eax+FIBER_CONTEXT_EDI], edi
mov [eax+FIBER_CONTEXT_EBP], ebp
- /* Save the return address */
- mov ebx, [esp]
- mov [eax+FIBER_CONTEXT_EIP], ebx
-
/* Check if we're to save FPU State */
cmp dword ptr [eax+FIBER_CONTEXT_FLAGS], CONTEXT_FULL OR CONTEXT_FLOATING_POINT
jnz NoFpuStateSave
/* Save the FPU State (Status and Control)*/
fstsw [eax+FIBER_CONTEXT_FLOAT_SAVE_STATUS_WORD]
- fstcw [eax+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]
+ fnstcw [eax+FIBER_CONTEXT_FLOAT_SAVE_CONTROL_WORD]
/* Check if the CPU supports SIMD MXCSR State Save */
- cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 0
+ cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 1
jnz NoFpuStateSave
stmxcsr [eax+FIBER_CONTEXT_DR6]
@@ -103,7 +99,7 @@
ControlWordEqual:
/* Load the new one */
- cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 0
+ cmp byte ptr ds:[PROCESSOR_FEATURE_FXSR], 1
jnz NoFpuStateRestore
ldmxcsr [ecx+FIBER_CONTEXT_DR6]
@@ -121,7 +117,8 @@
mov [edx+TEB_FLS_DATA], eax
/* Jump to new fiber */
- jmp dword ptr [ecx+FIBER_CONTEXT_EIP]
+ mov esp, [ecx+FIBER_CONTEXT_ESP]
+ ret 4
+END
-END
/* EOF */