From 53e621c1750de1ae7210017af72afa22abedb279 Mon Sep 17 00:00:00 2001 From: Duncan Ogilvie Date: Sun, 4 Mar 2018 22:04:04 +0100 Subject: [PATCH] DBG: replace WAITID_STOP with a more reliable method closes #1852 --- src/dbg/commands/cmd-debug-control.cpp | 67 ++++++++++++++++++++------ src/dbg/debugger.cpp | 11 +---- src/dbg/debugger.h | 1 + src/dbg/threading.h | 1 - src/dbg/x64dbg.cpp | 15 ++---- 5 files changed, 59 insertions(+), 36 deletions(-) diff --git a/src/dbg/commands/cmd-debug-control.cpp b/src/dbg/commands/cmd-debug-control.cpp index c150fe8a..1479fdb7 100644 --- a/src/dbg/commands/cmd-debug-control.cpp +++ b/src/dbg/commands/cmd-debug-control.cpp @@ -13,6 +13,8 @@ #include "handle.h" #include "thread.h" #include "GetPeArch.h" +#include "database.h" +#include "exception.h" static bool skipInt3Stepping(int argc, char* argv[]) { @@ -50,11 +52,14 @@ bool cbDebugRunInternal(int argc, char* argv[]) bool cbDebugInit(int argc, char* argv[]) { - cbDebugStop(argc, argv); - - static char arg1[deflen] = ""; if(IsArgumentsLessThan(argc, 2)) return false; + + EXCLUSIVE_ACQUIRE(LockDebugStartStop); + cbDebugStop(argc, argv); + ASSERT_TRUE(hDebugLoopThread == nullptr); + + static char arg1[deflen] = ""; strcpy_s(arg1, argv[1]); wchar_t szResolvedPath[MAX_PATH] = L""; if(ResolveShortcut(GuiGetWindowHandle(), StringUtils::Utf8ToUtf16(arg1).c_str(), szResolvedPath, _countof(szResolvedPath))) @@ -132,12 +137,21 @@ bool cbDebugInit(int argc, char* argv[]) init.commandline = commandline; if(*currentfolder) init.currentfolder = currentfolder; - CloseHandle(CreateThread(0, 0, threadDebugLoop, &init, 0, 0)); + + hDebugLoopThread = CreateThread(nullptr, 0, threadDebugLoop, &init, CREATE_SUSPENDED, nullptr); + ResumeThread(hDebugLoopThread); return true; } bool cbDebugStop(int argc, char* argv[]) { + EXCLUSIVE_ACQUIRE(LockDebugStartStop); + if(!hDebugLoopThread) + return false; + + auto hDebugLoopThreadCopy = hDebugLoopThread; + hDebugLoopThread = nullptr; + // HACK: TODO: Don't kill script on debugger ending a process //scriptreset(); //reset the currently-loaded script _dbg_animatestop(); @@ -145,20 +159,37 @@ bool cbDebugStop(int argc, char* argv[]) //history HistoryClear(); DWORD BeginTick = GetTickCount(); - while(waitislocked(WAITID_STOP)) //custom waiting + + while(true) { - unlock(WAITID_RUN); - Sleep(100); - DWORD CurrentTick = GetTickCount(); - if(CurrentTick - BeginTick > 10000) + switch(WaitForSingleObject(hDebugLoopThreadCopy, 100)) { - dputs(QT_TRANSLATE_NOOP("DBG", "The debuggee does not stop after 10 seconds. The debugger state may be corrupted.")); + case WAIT_OBJECT_0: + CloseHandle(hDebugLoopThreadCopy); + return true; + + case WAIT_TIMEOUT: + { + unlock(WAITID_RUN); + DWORD CurrentTick = GetTickCount(); + if(CurrentTick - BeginTick > 10000) + { + dputs(QT_TRANSLATE_NOOP("DBG", "The debuggee does not stop after 10 seconds. The debugger state may be corrupted.")); + DbSave(DbLoadSaveType::All); + TerminateThread(hDebugLoopThreadCopy, 1); // TODO: this will lose state and cause possible corruption if a critical section is still owned + CloseHandle(hDebugLoopThreadCopy); + return false; + } + if(CurrentTick - BeginTick >= 300) + TerminateProcess(fdProcessInfo->hProcess, -1); + } + break; + + case WAIT_FAILED: + dprintf_untranslated("WAIT_FAILED, GetLastError() = %d (%s)\n", GetLastError(), ErrorCodeToName(GetLastError()).c_str()); return false; } - if(CurrentTick - BeginTick >= 300) - TerminateProcess(fdProcessInfo->hProcess, -1); } - return true; } bool cbDebugAttach(int argc, char* argv[]) @@ -168,8 +199,11 @@ bool cbDebugAttach(int argc, char* argv[]) duint pid = 0; if(!valfromstring(argv[1], &pid, false)) return false; - if(DbgIsDebugging()) - DbgCmdExecDirect("stop"); + + EXCLUSIVE_ACQUIRE(LockDebugStartStop); + cbDebugStop(argc, argv); + ASSERT_TRUE(hDebugLoopThread == nullptr); + Handle hProcess = TitanOpenProcess(PROCESS_ALL_ACCESS, false, (DWORD)pid); if(!hProcess) { @@ -212,7 +246,8 @@ bool cbDebugAttach(int argc, char* argv[]) if(tid) dbgsetresumetid(tid); } - CloseHandle(CreateThread(0, 0, threadAttachLoop, (void*)pid, 0, 0)); + hDebugLoopThread = CreateThread(nullptr, 0, threadAttachLoop, (void*)pid, CREATE_SUSPENDED, nullptr); + ResumeThread(hDebugLoopThread); return true; } diff --git a/src/dbg/debugger.cpp b/src/dbg/debugger.cpp index 5266d394..e38f92f0 100644 --- a/src/dbg/debugger.cpp +++ b/src/dbg/debugger.cpp @@ -91,6 +91,7 @@ duint maxSkipExceptionCount = 10000; HANDLE mProcHandle; HANDLE mForegroundHandle; duint mRtrPreviousCSP = 0; +HANDLE hDebugLoopThread = nullptr; static duint dbgcleartracestate() { @@ -2543,10 +2544,6 @@ void dbgstartscriptthread(CBPLUGINSCRIPT cbScript) static void debugLoopFunction(void* lpParameter, bool attach) { - //we are running - EXCLUSIVE_ACQUIRE(LockDebugStartStop); - lock(WAITID_STOP); - //initialize variables bIsAttached = attach; dbgsetskipexceptions(false); @@ -2612,7 +2609,6 @@ static void debugLoopFunction(void* lpParameter, bool attach) if(answer == IDYES && dbgrestartadmin()) { fdProcessInfo = &g_pi; - unlock(WAITID_STOP); GuiCloseApplication(); return; } @@ -2625,7 +2621,6 @@ static void debugLoopFunction(void* lpParameter, bool attach) error += ", uiAccess=\"true\""; } fdProcessInfo = &g_pi; - unlock(WAITID_STOP); dprintf(QT_TRANSLATE_NOOP("DBG", "Error starting process (CreateProcess, %s)!\n"), error.c_str()); return; } @@ -2636,7 +2631,6 @@ static void debugLoopFunction(void* lpParameter, bool attach) { dputs(QT_TRANSLATE_NOOP("DBG", "IsWow64Process failed!")); StopDebug(); - unlock(WAITID_STOP); return; } if((mewow64 && !wow64) || (!mewow64 && wow64)) @@ -2646,7 +2640,6 @@ static void debugLoopFunction(void* lpParameter, bool attach) #else dputs(QT_TRANSLATE_NOOP("DBG", "Use x64dbg to debug this process!")); #endif // _WIN64 - unlock(WAITID_STOP); return; } @@ -2750,7 +2743,7 @@ static void debugLoopFunction(void* lpParameter, bool attach) varset("$pid", (duint)0, true); if(hProcessToken) CloseHandle(hProcessToken); - unlock(WAITID_STOP); //we are done + pDebuggedEntry = 0; pDebuggedBase = 0; pCreateProcessBase = 0; diff --git a/src/dbg/debugger.h b/src/dbg/debugger.h index e15cad02..ebf00b62 100644 --- a/src/dbg/debugger.h +++ b/src/dbg/debugger.h @@ -127,5 +127,6 @@ extern duint maxSkipExceptionCount; extern HANDLE mProcHandle; extern HANDLE mForegroundHandle; extern duint mRtrPreviousCSP; +extern HANDLE hDebugLoopThread; #endif // _DEBUGGER_H diff --git a/src/dbg/threading.h b/src/dbg/threading.h index 195e432e..a1ea7eed 100644 --- a/src/dbg/threading.h +++ b/src/dbg/threading.h @@ -6,7 +6,6 @@ enum WAIT_ID { WAITID_RUN, - WAITID_STOP, WAITID_LAST }; diff --git a/src/dbg/x64dbg.cpp b/src/dbg/x64dbg.cpp index 06f74667..625b278f 100644 --- a/src/dbg/x64dbg.cpp +++ b/src/dbg/x64dbg.cpp @@ -752,21 +752,16 @@ extern "C" DLL_EXPORT const char* _dbg_dbginit() */ extern "C" DLL_EXPORT void _dbg_dbgexitsignal() { + dputs(QT_TRANSLATE_NOOP("DBG", "Stopping command thread...")); + bStopCommandLoopThread = true; + MsgFreeStack(gMsgStack); + WaitForThreadTermination(hCommandLoopThread); dputs(QT_TRANSLATE_NOOP("DBG", "Stopping running debuggee...")); - cbDebugStop(0, 0); - dputs(QT_TRANSLATE_NOOP("DBG", "Waiting for the debuggee to be stopped...")); - if(!waitfor(WAITID_STOP, 10000)) //after this, debugging stopped - { - dputs(QT_TRANSLATE_NOOP("DBG", "The debuggee does not close after 10 seconds. Probably the debugger state has been corrupted.")); - } + cbDebugStop(0, 0); //after this, debugging stopped dputs(QT_TRANSLATE_NOOP("DBG", "Aborting scripts...")); scriptabort(); dputs(QT_TRANSLATE_NOOP("DBG", "Unloading plugins...")); pluginunloadall(); - dputs(QT_TRANSLATE_NOOP("DBG", "Stopping command thread...")); - bStopCommandLoopThread = true; - MsgFreeStack(gMsgStack); - WaitForThreadTermination(hCommandLoopThread, 10000); dputs(QT_TRANSLATE_NOOP("DBG", "Cleaning up allocated data...")); cmdfree(); varfree();