diff --git a/GleeBug/Debugger.Loop.Exception.cpp b/GleeBug/Debugger.Loop.Exception.cpp index 1a430d5..e8c0bb0 100644 --- a/GleeBug/Debugger.Loop.Exception.cpp +++ b/GleeBug/Debugger.Loop.Exception.cpp @@ -5,6 +5,8 @@ namespace GleeBug { void Debugger::exceptionBreakpoint(const EXCEPTION_RECORD & exceptionRecord, const bool firstChance) { + std::unique_lock lock(mProcess->breakpointMutex); + //check if the breakpoint exists auto exceptionAddress = ptr(exceptionRecord.ExceptionAddress); auto foundInfo = mProcess->breakpoints.find({ BreakpointType::Software, exceptionAddress }); @@ -17,6 +19,7 @@ namespace GleeBug mContinueStatus = DBG_CONTINUE; //call the callback + lock.unlock(); cbSystemBreakpoint(); } else @@ -50,6 +53,8 @@ namespace GleeBug } mProcess->StepInternal([this, info]() { + std::lock_guard lock(mProcess->breakpointMutex); + //only restore the bytes if the breakpoint still exists auto foundBreakpoint = mProcess->breakpoints.find({ BreakpointType::Software, info.address }); if(foundBreakpoint != mProcess->breakpoints.end()) @@ -64,13 +69,18 @@ namespace GleeBug } }); + BreakpointCallback breakpointCallback; + auto foundCallback = mProcess->breakpointCallbacks.find({ BreakpointType::Software, info.address }); + if(foundCallback != mProcess->breakpointCallbacks.end()) + breakpointCallback = foundCallback->second; + lock.unlock(); + //call the generic callback cbBreakpoint(info); //call the user callback - auto foundCallback = mProcess->breakpointCallbacks.find({ BreakpointType::Software, info.address }); - if(foundCallback != mProcess->breakpointCallbacks.end()) - foundCallback->second(info); + if(breakpointCallback) + breakpointCallback(info); //delete the breakpoint if it is singleshoot if(info.singleshoot) @@ -85,9 +95,6 @@ namespace GleeBug mThread->isInternalStepping = false; mContinueStatus = DBG_CONTINUE; - // Internal step callbacks can re-arm a memory-breakpoint page. Serialize - // that map access with concurrent set/delete transactions. - std::lock_guard lock(mProcess->memoryBreakpointMutex); mThread->cbInternalStep(); } if(mThread->isSingleStepping) //handle single step @@ -142,6 +149,7 @@ namespace GleeBug return; //not a hardware breakpoint //find the breakpoint in the internal structures + std::unique_lock lock(mProcess->breakpointMutex); auto foundInfo = mProcess->breakpoints.find({ BreakpointType::Hardware, breakpointAddress }); if(foundInfo == mProcess->breakpoints.end()) return; //not a valid hardware breakpoint @@ -156,21 +164,31 @@ namespace GleeBug mThread->DeleteHardwareBreakpoint(breakpointSlot); mProcess->StepInternal([this, info]() { + std::lock_guard lock(mProcess->breakpointMutex); + //only restore if the breakpoint still exists if(mProcess->breakpoints.find({ BreakpointType::Hardware, info.address }) != mProcess->breakpoints.end()) mThread->SetHardwareBreakpoint(info.address, info.internal.hardware.slot, info.internal.hardware.type, info.internal.hardware.size); }); + BreakpointCallback breakpointCallback; + auto foundCallback = mProcess->breakpointCallbacks.find({ BreakpointType::Hardware, info.address }); + if(foundCallback != mProcess->breakpointCallbacks.end()) + breakpointCallback = foundCallback->second; + lock.unlock(); + //call the generic callback cbBreakpoint(info); //call the user callback - auto foundCallback = mProcess->breakpointCallbacks.find({ BreakpointType::Hardware, info.address }); - if(foundCallback != mProcess->breakpointCallbacks.end()) - foundCallback->second(info); + if(breakpointCallback) + breakpointCallback(info); //if the breakpoint was deleted during callback, clear internal stepping to prevent thread suspension - if(mProcess->breakpoints.find({ BreakpointType::Hardware, info.address }) == mProcess->breakpoints.end()) + lock.lock(); + const bool breakpointDeleted = mProcess->breakpoints.find({ BreakpointType::Hardware, info.address }) == mProcess->breakpoints.end(); + lock.unlock(); + if(breakpointDeleted) { mThread->isInternalStepping = false; Registers(mThread->hThread, CONTEXT_CONTROL).TrapFlag = false; @@ -185,13 +203,22 @@ namespace GleeBug { // Page protections are changed before set/delete publishes its metadata. // Wait for the transaction before classifying this memory-breakpoint fault. - std::unique_lock lock(mProcess->memoryBreakpointMutex); + std::unique_lock lock(mProcess->breakpointMutex); /* ASSUME: exceptionAddress may or may not have been generated by your breakpoints. */ char error[128] = ""; + auto reportError = [&]() + { + const bool relock = lock.owns_lock(); + if(relock) + lock.unlock(); + cbInternalError(error); + if(relock) + lock.lock(); + }; auto exceptionAddress = ptr(exceptionRecord.ExceptionInformation[1]); //check if the exception address is directly in the range of a memory breakpoint @@ -212,7 +239,7 @@ namespace GleeBug if(!mProcess->MemProtect(foundPage->first, PAGE_SIZE, foundPage->second.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)foundPage->first); - cbInternalError(error); + reportError(); } //However the following situations may occur: @@ -223,6 +250,8 @@ namespace GleeBug // then we ought to restore the protection. mProcess->StepInternal([this, pBaseAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + //seek out the page address auto found_page = mProcess->memoryBreakpointPages.find(pBaseAddr); if(found_page == mProcess->memoryBreakpointPages.end()) @@ -245,7 +274,7 @@ namespace GleeBug if(foundInfo == mProcess->breakpoints.end()) { sprintf_s(error, "inconsistent memory breakpoint at 0x%p", (void*)exceptionAddress); - cbInternalError(error); + reportError(); return; } @@ -260,7 +289,7 @@ namespace GleeBug if(bpxPage == mProcess->memoryBreakpointPages.end()) { sprintf_s(error, "Process::memoryBreakPointPages data structure is incosistent, should dump page at 0x%p", (void*)(exceptionAddress & ~(PAGE_SIZE - 1))); - cbInternalError(error); + reportError(); return; } auto pageAddr = bpxPage->first; @@ -284,11 +313,13 @@ namespace GleeBug if(!mProcess->MemProtect(pageAddr, PAGE_SIZE, pageProperties.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)pageAddr); - cbInternalError(error); + reportError(); } mProcess->StepInternal([this, pageAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + auto found_page = mProcess->memoryBreakpointPages.find(pageAddr); if(found_page == mProcess->memoryBreakpointPages.end()) return; @@ -328,11 +359,13 @@ namespace GleeBug if(!mProcess->MemProtect(pageAddr, PAGE_SIZE, pageProperties.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)pageAddr); - cbInternalError(error); + reportError(); } //Pass info as well mProcess->StepInternal([this, pageAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + //With page check this should work better: So when we reach this part of the code we are sure that: //-The exception Address In deed corresponded to an existing (now possibly deleted) memory breakpoint range //-memoryBreakpointPages was in deed consistent with this memory address that generated the exception (The data structure wasn't corrupted somehow) @@ -364,13 +397,22 @@ namespace GleeBug void Debugger::exceptionAccessViolation(const EXCEPTION_RECORD & exceptionRecord, bool firstChance) { - std::unique_lock lock(mProcess->memoryBreakpointMutex); + std::unique_lock lock(mProcess->breakpointMutex); /* ASSUME: exceptionAddress may or may not have been generated by your breakpoints. */ char error[128] = ""; + auto reportError = [&]() + { + const bool relock = lock.owns_lock(); + if(relock) + lock.unlock(); + cbInternalError(error); + if(relock) + lock.lock(); + }; auto exceptionAddress = ptr(exceptionRecord.ExceptionInformation[1]); //check if the exception address is directly in the range of a memory breakpoint @@ -391,7 +433,7 @@ namespace GleeBug if(!mProcess->MemProtect(foundPage->first, PAGE_SIZE, foundPage->second.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)foundPage->first); - cbInternalError(error); + reportError(); } //However the following situations may occur: @@ -402,6 +444,8 @@ namespace GleeBug // then we ought to restore the protection. mProcess->StepInternal([this, pBaseAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + //seek out the page address auto found_page = mProcess->memoryBreakpointPages.find(pBaseAddr); if(found_page == mProcess->memoryBreakpointPages.end()) @@ -424,7 +468,7 @@ namespace GleeBug if(foundInfo == mProcess->breakpoints.end()) { sprintf_s(error, "inconsistent memory breakpoint at 0x%p", (void*)exceptionAddress); - cbInternalError(error); + reportError(); return; } @@ -439,7 +483,7 @@ namespace GleeBug if(bpxPage == mProcess->memoryBreakpointPages.end()) { sprintf_s(error, "Process::memoryBreakPointPages data structure is incosistent, should dump page at 0x%p", (void*)(exceptionAddress & ~(PAGE_SIZE - 1))); - cbInternalError(error); + reportError(); return; } auto pageAddr = bpxPage->first; @@ -474,7 +518,7 @@ namespace GleeBug if(!mProcess->MemProtect(pageAddr, PAGE_SIZE, pageProperties.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)pageAddr); - cbInternalError(error); + reportError(); } // The page-wide protection belongs to another range. Execute this @@ -482,6 +526,8 @@ namespace GleeBug // any memory breakpoint still owns it. mProcess->StepInternal([this, pageAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + auto foundPage = mProcess->memoryBreakpointPages.find(pageAddr); if(foundPage != mProcess->memoryBreakpointPages.end()) mProcess->MemProtect(pageAddr, PAGE_SIZE, foundPage->second.NewProtect); @@ -513,11 +559,13 @@ namespace GleeBug if(!mProcess->MemProtect(pageAddr, PAGE_SIZE, pageProperties.OldProtect)) { sprintf_s(error, "MemProtect failed on 0x%p", (void*)pageAddr); - cbInternalError(error); + reportError(); } //Pass info as well mProcess->StepInternal([this, pageAddr]() { + std::lock_guard lock(mProcess->breakpointMutex); + //With page check this should work better: So when we reach this part of the code we are sure that: //-The exception Address In deed corresponded to an existing (now possibly deleted) memory breakpoint range //-memoryBreakpointPages was in deed consistent with this memory address that generated the exception (The data structure wasn't corrupted somehow) diff --git a/GleeBug/Debugger.Loop.cpp b/GleeBug/Debugger.Loop.cpp index 3b089e2..19614f0 100644 --- a/GleeBug/Debugger.Loop.cpp +++ b/GleeBug/Debugger.Loop.cpp @@ -75,7 +75,10 @@ namespace GleeBug // single-step event arrives. consecutiveTimeouts++; if(consecutiveTimeouts >= 2 && ThreadBeingProcessed == 0 && SuspendedThreads.empty() && DeferredExceptionThreads.empty() && mProcess) + { + std::lock_guard lock(mProcess->breakpointMutex); mProcess->recentlyDeletedSwbp.clear(); + } continue; } } diff --git a/GleeBug/Debugger.Process.Breakpoint.cpp b/GleeBug/Debugger.Process.Breakpoint.cpp index 5b114c2..b8e1b56 100644 --- a/GleeBug/Debugger.Process.Breakpoint.cpp +++ b/GleeBug/Debugger.Process.Breakpoint.cpp @@ -4,6 +4,8 @@ namespace GleeBug { bool Process::SetBreakpoint(ptr address, bool singleshoot, SoftwareType type) { + std::lock_guard lock(breakpointMutex); + //check the address if(!MemIsValidPtr(address) || breakpoints.find({ BreakpointType::Software, address }) != breakpoints.end()) @@ -44,6 +46,8 @@ namespace GleeBug bool Process::SetBreakpoint(ptr address, const BreakpointCallback & cbBreakpoint, bool singleshoot, SoftwareType type) { + std::lock_guard lock(breakpointMutex); + //check if a callback on this address was already found if(breakpointCallbacks.find({ BreakpointType::Software, address }) != breakpointCallbacks.end()) return false; @@ -57,6 +61,8 @@ namespace GleeBug bool Process::DeleteBreakpoint(ptr address) { + std::lock_guard lock(breakpointMutex); + //find the breakpoint auto found = breakpoints.find({ BreakpointType::Software, address }); if(found == breakpoints.end()) @@ -79,6 +85,8 @@ namespace GleeBug bool Process::GetFreeHardwareBreakpointSlot(HardwareSlot & slot) const { + std::lock_guard lock(breakpointMutex); + //find a free hardware breakpoint slot for(int i = 0; i < HWBP_COUNT; i++) { @@ -93,6 +101,8 @@ namespace GleeBug bool Process::SetHardwareBreakpoint(ptr address, HardwareSlot slot, HardwareType type, HardwareSize size, bool singleshoot) { + std::lock_guard lock(breakpointMutex); + //check the address if(!MemIsValidPtr(address) || breakpoints.find({ BreakpointType::Hardware, address }) != breakpoints.end()) @@ -138,6 +148,8 @@ namespace GleeBug bool Process::SetHardwareBreakpoint(ptr address, HardwareSlot slot, const BreakpointCallback & cbBreakpoint, HardwareType type, HardwareSize size, bool singleshoot) { + std::lock_guard lock(breakpointMutex); + //check if a callback on this address was already found if(breakpointCallbacks.find({ BreakpointType::Hardware, address }) != breakpointCallbacks.end()) return false; @@ -151,6 +163,8 @@ namespace GleeBug bool Process::DeleteHardwareBreakpoint(ptr address) { + std::lock_guard lock(breakpointMutex); + //find the hardware breakpoint auto found = breakpoints.find({ BreakpointType::Hardware, address }); if(found == breakpoints.end()) @@ -369,7 +383,7 @@ namespace GleeBug bool Process::SetMemoryBreakpoint(ptr address, ptr size, MemoryType type, bool singleshoot) { - std::lock_guard lock(memoryBreakpointMutex); + std::lock_guard lock(breakpointMutex); DPRINTF(); // Basic checks, including the range-end overflow that would otherwise wrap @@ -458,7 +472,7 @@ namespace GleeBug bool Process::SetMemoryBreakpoint(ptr address, ptr size, const BreakpointCallback & cbBreakpoint, MemoryType type, bool singleshoot) { - std::lock_guard lock(memoryBreakpointMutex); + std::lock_guard lock(breakpointMutex); //check if a callback on this address was already found if(breakpointCallbacks.find({ BreakpointType::Memory, address }) != breakpointCallbacks.end()) @@ -473,7 +487,7 @@ namespace GleeBug bool Process::DeleteMemoryBreakpoint(ptr address) { - std::lock_guard lock(memoryBreakpointMutex); + std::lock_guard lock(breakpointMutex); // Find the byte range containing address, then find its breakpoint record. const auto range = memoryBreakpointRanges.find(Range(address, address)); diff --git a/GleeBug/Debugger.Process.Memory.cpp b/GleeBug/Debugger.Process.Memory.cpp index 6fe01f5..6c8b802 100644 --- a/GleeBug/Debugger.Process.Memory.cpp +++ b/GleeBug/Debugger.Process.Memory.cpp @@ -13,6 +13,8 @@ namespace GleeBug bool Process::MemReadSafe(ptr address, void* buffer, ptr size, ptr* bytesRead) const { + std::lock_guard lock(breakpointMutex); + if(!MemReadUnsafe(address, buffer, size, bytesRead)) return false; @@ -65,6 +67,8 @@ namespace GleeBug bool Process::MemWriteSafe(ptr address, const void* buffer, ptr size, ptr* bytesWritten) { + std::lock_guard lock(breakpointMutex); + if(size == 0) { if(bytesWritten) diff --git a/GleeBug/Debugger.Process.h b/GleeBug/Debugger.Process.h index 19520df..3c2c23a 100644 --- a/GleeBug/Debugger.Process.h +++ b/GleeBug/Debugger.Process.h @@ -31,7 +31,7 @@ namespace GleeBug BreakpointInfo hardwareBreakpoints[4]; MemoryBreakpointSet memoryBreakpointRanges; MemoryBreakpointMap memoryBreakpointPages; - std::recursive_mutex memoryBreakpointMutex; + mutable std::recursive_mutex breakpointMutex; std::unordered_set recentlyDeletedSwbp; diff --git a/GleeBug/Debugger.cpp b/GleeBug/Debugger.cpp index a987302..ebd5167 100644 --- a/GleeBug/Debugger.cpp +++ b/GleeBug/Debugger.cpp @@ -116,6 +116,8 @@ retry_no_aslr: if(mProcess) { + std::lock_guard lock(mProcess->breakpointMutex); + // 1. Restore all software (INT3) breakpoints, otherwise the debuggee // faults on a leftover 0xCC once it is no longer being debugged. for(auto it = mProcess->breakpoints.begin(); it != mProcess->breakpoints.end(); ) diff --git a/TitanEngineEmulator/Emulator.h b/TitanEngineEmulator/Emulator.h index 84ed928..7b793d8 100644 --- a/TitanEngineEmulator/Emulator.h +++ b/TitanEngineEmulator/Emulator.h @@ -793,6 +793,7 @@ public: bool IsBPXEnabled(ULONG_PTR bpxAddress) { + std::lock_guard lock(mProcess->breakpointMutex); return (mProcess->MemIsValidPtr(bpxAddress) && mProcess->breakpoints.find({ BreakpointType::Software, bpxAddress }) != mProcess->breakpoints.end()); } @@ -854,7 +855,11 @@ public: auto slot = IndexOfRegister - UE_DR0; if(!mProcess || slot > 3) return false; - auto address = mProcess->hardwareBreakpoints[slot].address; + ptr address; + { + std::lock_guard lock(mProcess->breakpointMutex); + address = mProcess->hardwareBreakpoints[slot].address; + } return mProcess->DeleteHardwareBreakpoint(address); } @@ -874,7 +879,11 @@ public: { for(auto & it : mProcesses) { - auto breakpoints = it.second->breakpoints; //explicit copy + BreakpointMap breakpoints; + { + std::lock_guard lock(it.second->breakpointMutex); + breakpoints = it.second->breakpoints; + } for(const auto & jt : breakpoints) it.second->DeleteGenericBreakpoint(jt.second); }