[ion] 52813: [KERNEL32]: User-mode Debugging fixes. Bug: RemoveHandles was checking for process AND thread handle match, instead of OR. Bug: CloseAllProcessHandles and RemoveHandles's processing lo...
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/d... ============================================================================== --- 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 */
-
ion＠svn.reactos.org