From 99d3c72b3f3863444cef20f784efda16b93efde0 Mon Sep 17 00:00:00 2001 From: Duncan Ogilvie Date: Thu, 16 Jul 2026 13:24:59 +0200 Subject: [PATCH] Reimplement setting of memory breakpoints to work better on large ranges --- GleeBug/Debugger.Breakpoint.h | 15 +- GleeBug/Debugger.Loop.Exception.cpp | 94 ++++-- GleeBug/Debugger.Process.Breakpoint.cpp | 376 ++++++++++++++++-------- GleeBug/Debugger.Process.h | 10 +- GleeBug/Debugger.cpp | 73 ++++- GleeBug/GleeBug.h | 1 + 6 files changed, 408 insertions(+), 161 deletions(-) diff --git a/GleeBug/Debugger.Breakpoint.h b/GleeBug/Debugger.Breakpoint.h index 7bdc553..805b7be 100644 --- a/GleeBug/Debugger.Breakpoint.h +++ b/GleeBug/Debugger.Breakpoint.h @@ -95,10 +95,17 @@ namespace GleeBug */ struct MemoryBreakpointData { - uint32 Refcount; - uint32 Type; - DWORD OldProtect; - DWORD NewProtect; + // Refcount and Type are cached aggregates used by existing page-handling + // code. Per-type counts preserve multiplicity when same-type ranges share + // one page and allow deletion to derive the exact remaining protection. + uint32 Refcount = 0; + uint32 Type = 0; + uint32 AccessRefs = 0; + uint32 ReadRefs = 0; + uint32 WriteRefs = 0; + uint32 ExecuteRefs = 0; + DWORD OldProtect = 0; + DWORD NewProtect = 0; }; }; diff --git a/GleeBug/Debugger.Loop.Exception.cpp b/GleeBug/Debugger.Loop.Exception.cpp index f887417..1a430d5 100644 --- a/GleeBug/Debugger.Loop.Exception.cpp +++ b/GleeBug/Debugger.Loop.Exception.cpp @@ -85,7 +85,9 @@ namespace GleeBug mThread->isInternalStepping = false; mContinueStatus = DBG_CONTINUE; - //call the internal step callback + // 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 @@ -181,6 +183,10 @@ namespace GleeBug void Debugger::exceptionGuardPage(const EXCEPTION_RECORD & exceptionRecord, bool firstChance) { + // 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); + /* ASSUME: exceptionAddress may or may not have been generated by your breakpoints. @@ -260,14 +266,15 @@ namespace GleeBug auto pageAddr = bpxPage->first; auto pageProperties = bpxPage->second; - // PAGE_GUARD is shared by access/read/write/execute breakpoints, so the exception type in - // ExceptionInformation[0] decides whether this guard-page fault actually belongs to this breakpoint. - // Access breakpoints intentionally match every access type. + // PAGE_GUARD is shared by every breakpoint range on this page. Match against the + // range that contains the accessed byte; the aggregate page type may include a + // different range and must only be used to derive the page protection. const auto accessType = exceptionRecord.ExceptionInformation[0]; - const auto isAccessBreakpoint = (pageProperties.Type & 0x1) != 0; - const auto matchesRead = accessType == 0 && (((pageProperties.Type & 0x2) != 0) || isAccessBreakpoint); - const auto matchesWrite = accessType == 1 && (((pageProperties.Type & 0x4) != 0) || isAccessBreakpoint); - const auto matchesExecute = accessType == 8 && (((pageProperties.Type & 0x8) != 0) || isAccessBreakpoint); + const auto breakpointType = info.internal.memory.type; + const auto isAccessBreakpoint = breakpointType == MemoryType::Access; + const auto matchesRead = accessType == 0 && (breakpointType == MemoryType::Read || isAccessBreakpoint); + const auto matchesWrite = accessType == 1 && (breakpointType == MemoryType::Write || isAccessBreakpoint); + const auto matchesExecute = accessType == 8 && (breakpointType == MemoryType::Execute || isAccessBreakpoint); if(!matchesRead && !matchesWrite && !matchesExecute) { mContinueStatus = DBG_CONTINUE; @@ -297,16 +304,22 @@ namespace GleeBug The breakpoint at exceptionAddress was indeed generated by me. Its safe to call the callbacks. */ + // Copy the callback while the maps are locked, then release the lock before + // notifying x64dbg. Breakpoint callbacks can synchronously add or delete + // breakpoints and must not block behind the transaction lock we hold here. + BreakpointCallback breakpointCallback; + const auto bpxCb = mProcess->breakpointCallbacks.find({ BreakpointType::Memory, info.address }); + if(bpxCb != mProcess->breakpointCallbacks.end()) + breakpointCallback = bpxCb->second; + lock.unlock(); + //generic breakpoint callback function. cbBreakpoint(info); //TODO: execute the user callback (if present) //FIXED: - auto bpxCb = mProcess->breakpointCallbacks.find({ BreakpointType::Memory, info.address }); - if(bpxCb != mProcess->breakpointCallbacks.end()) - { - bpxCb->second(info); - } + if(breakpointCallback) + breakpointCallback(info); mContinueStatus = DBG_CONTINUE; @@ -351,6 +364,8 @@ namespace GleeBug void Debugger::exceptionAccessViolation(const EXCEPTION_RECORD & exceptionRecord, bool firstChance) { + std::unique_lock lock(mProcess->memoryBreakpointMutex); + /* ASSUME: exceptionAddress may or may not have been generated by your breakpoints. @@ -436,33 +451,60 @@ namespace GleeBug Write = 4, Execute = 8 */ - //ExceptionInformation[0] should be considered as 1 or 8, because these are for exceptions generated on write or execute. - //Execute is only implemented with page guard if no Data-Execution-Prevention is implemented by the Kernel. - if((exceptionRecord.ExceptionInformation[0] == 1) && (!(pageProperties.Type & 4))) - { - //The exception was on Write but there was no page breakpoint in Write? Then the program changed the page permissions, or naturally overwritten protected data. We do not interfere. + // Access violations generated by memory breakpoints are write or execute faults. + // First verify that the aggregate page state could have caused this fault. A + // debuggee-generated violation must still be passed to the debuggee. + const auto accessType = exceptionRecord.ExceptionInformation[0]; + const bool pageBreakpointCausedFault = + (accessType == 1 && pageProperties.WriteRefs != 0) || + (accessType == 8 && pageProperties.ExecuteRefs != 0); + if(!pageBreakpointCausedFault) return; - } - if((exceptionRecord.ExceptionInformation[0] == 8) && (!(pageProperties.Type & 8))) + // Several byte-disjoint ranges can share this page. Only invoke the callback + // when the range containing the accessed byte matches the actual access type. + const auto breakpointType = info.internal.memory.type; + const bool isAccessBreakpoint = breakpointType == MemoryType::Access; + const bool matchesBreakpoint = + (accessType == 1 && (breakpointType == MemoryType::Write || isAccessBreakpoint)) || + (accessType == 8 && (breakpointType == MemoryType::Execute || isAccessBreakpoint)); + if(!matchesBreakpoint) { - //The exception was on Execution but there was no page breakpoint in Execute? Then the program changed the page permissions, or naturally executed protected code. We do not interfere. + mContinueStatus = DBG_CONTINUE; + if(!mProcess->MemProtect(pageAddr, PAGE_SIZE, pageProperties.OldProtect)) + { + sprintf_s(error, "MemProtect failed on 0x%p", (void*)pageAddr); + cbInternalError(error); + } + + // The page-wide protection belongs to another range. Execute this + // instruction once with the original protection, then re-arm the page if + // any memory breakpoint still owns it. + mProcess->StepInternal([this, pageAddr]() + { + auto foundPage = mProcess->memoryBreakpointPages.find(pageAddr); + if(foundPage != mProcess->memoryBreakpointPages.end()) + mProcess->MemProtect(pageAddr, PAGE_SIZE, foundPage->second.NewProtect); + }); return; } /* ASSUME: The breakpoint at exceptionAddress was indeed generated by me. */ + BreakpointCallback breakpointCallback; + const auto bpxCb = mProcess->breakpointCallbacks.find({ BreakpointType::Memory, info.address }); + if(bpxCb != mProcess->breakpointCallbacks.end()) + breakpointCallback = bpxCb->second; + lock.unlock(); + //generic breakpoint callback function. cbBreakpoint(info); //TODO: execute the user callback (if present) //FIXED: - auto bpxCb = mProcess->breakpointCallbacks.find({ BreakpointType::Memory, info.address }); - if(bpxCb != mProcess->breakpointCallbacks.end()) - { - bpxCb->second(info); - } + if(breakpointCallback) + breakpointCallback(info); mContinueStatus = DBG_CONTINUE; diff --git a/GleeBug/Debugger.Process.Breakpoint.cpp b/GleeBug/Debugger.Process.Breakpoint.cpp index bb11f5c..5b114c2 100644 --- a/GleeBug/Debugger.Process.Breakpoint.cpp +++ b/GleeBug/Debugger.Process.Breakpoint.cpp @@ -230,132 +230,236 @@ namespace GleeBug } } - bool Process::SetNewPageProtection(ptr page, MemoryBreakpointData & data, MemoryType type) + // Derive the page-wide protection from per-type reference counts. Byte-disjoint + // breakpoint ranges may share a page, including several ranges of the same type. + static DWORD MemoryBreakpointProtection(const MemoryBreakpointData & data, bool permanentDep) { - DPRINTF(); - //TODO: handle PAGE_NOACCESS and such correctly (since it cannot be combined with PAGE_GUARD) + if(data.Refcount == 0) + return data.OldProtect; - auto found = memoryBreakpointPages.find(page); - if(found == memoryBreakpointPages.end()) + const bool needsGuard = data.AccessRefs != 0 || data.ReadRefs != 0 || (data.ExecuteRefs != 0 && !permanentDep); + if(needsGuard) { - data.Refcount = 1; - switch(type) - { - case MemoryType::Access: - case MemoryType::Read: - data.NewProtect = data.OldProtect | PAGE_GUARD; - break; - case MemoryType::Write: - data.NewProtect = RemoveWriteAccess(data.OldProtect); - break; - case MemoryType::Execute: - data.NewProtect = permanentDep ? RemoveExecuteAccess(data.OldProtect) : data.OldProtect | PAGE_GUARD; - break; - } - } - else - { - auto & oldData = found->second; - data.Type = oldData.Type | uint32(type); //combines new protection - data.OldProtect = oldData.OldProtect; // old protection remains the same - data.Refcount = oldData.Refcount + 1; //increment reference count - if(oldData.Type == uint32(type)) // Edge case for when you need to set a mem bpx on a same page with the same type, you just leave newProtect = OldProtect. - { - data.NewProtect = data.OldProtect; - } - else if(data.Type & uint32(MemoryType::Access) || data.Type & uint32(MemoryType::Read)) // Access/Read always becomes PAGE_GUARD ; This page cannot access or Read? - data.NewProtect = data.OldProtect | PAGE_GUARD; //as before - else if(data.Type & (uint32(MemoryType::Write) | uint32(MemoryType::Execute))) // Write + Execute becomes either PAGE_GUARD or both write and execute flags removed - data.NewProtect = permanentDep ? RemoveExecuteAccess(RemoveWriteAccess(data.OldProtect)) : data.OldProtect | PAGE_GUARD; + // PAGE_GUARD cannot be combined with PAGE_NOACCESS, PAGE_NOCACHE, or PAGE_WRITECOMBINE. + if((data.OldProtect & 0xFF) == PAGE_NOACCESS) + return (data.OldProtect & ~0x7FF) | PAGE_NOACCESS; + return (data.OldProtect & ~0x700) | PAGE_GUARD; } - dprintf("SetNewPageProtection(%p, %X)\n", page, data.NewProtect); - return MemProtect(page, PAGE_SIZE, data.NewProtect); + DWORD protect = data.OldProtect; + if(data.ExecuteRefs != 0) + protect = RemoveExecuteAccess(protect); + if(data.WriteRefs != 0) + protect = RemoveWriteAccess(protect); + return protect; + } + + static void RefreshMemoryBreakpointData(MemoryBreakpointData & data, bool permanentDep) + { + data.Refcount = data.AccessRefs + data.ReadRefs + data.WriteRefs + data.ExecuteRefs; + data.Type = 0; + if(data.AccessRefs != 0) + data.Type |= uint32(MemoryType::Access); + if(data.ReadRefs != 0) + data.Type |= uint32(MemoryType::Read); + if(data.WriteRefs != 0) + data.Type |= uint32(MemoryType::Write); + if(data.ExecuteRefs != 0) + data.Type |= uint32(MemoryType::Execute); + data.NewProtect = MemoryBreakpointProtection(data, permanentDep); + } + + static bool AddMemoryBreakpointReference(MemoryBreakpointData & data, MemoryType type, bool permanentDep) + { + uint32* refs = nullptr; + switch(type) + { + case MemoryType::Access: + refs = &data.AccessRefs; + break; + case MemoryType::Read: + refs = &data.ReadRefs; + break; + case MemoryType::Write: + refs = &data.WriteRefs; + break; + case MemoryType::Execute: + refs = &data.ExecuteRefs; + break; + } + if(refs == nullptr || *refs == ~uint32(0)) + return false; + ++*refs; + RefreshMemoryBreakpointData(data, permanentDep); + return true; + } + + static bool RemoveMemoryBreakpointReference(MemoryBreakpointData & data, MemoryType type, bool permanentDep) + { + uint32* refs = nullptr; + switch(type) + { + case MemoryType::Access: + refs = &data.AccessRefs; + break; + case MemoryType::Read: + refs = &data.ReadRefs; + break; + case MemoryType::Write: + refs = &data.WriteRefs; + break; + case MemoryType::Execute: + refs = &data.ExecuteRefs; + break; + } + if(refs == nullptr || *refs == 0) + return false; + --*refs; + RefreshMemoryBreakpointData(data, permanentDep); + return true; + } + + struct MemoryBreakpointPageAction + { + ptr page = 0; + ptr allocationBase = 0; + DWORD currentProtect = 0; + MemoryBreakpointData data; + }; + + static DWORD TargetProtection(const MemoryBreakpointPageAction & action) + { + return action.data.Refcount == 0 ? action.data.OldProtect : action.data.NewProtect; + } + + // Apply one VirtualProtectEx per maximal run with the same target protection. + // AllocationBase is part of the key because VirtualProtectEx cannot span + // independently reserved allocations even when their pages are consecutive. + static bool ApplyProtectionRuns(Process & process, const std::vector & actions, size_t count, bool rollback, size_t* appliedCount = nullptr) + { + size_t index = 0; + while(index < count) + { + const auto base = actions[index].page; + const auto allocationBase = actions[index].allocationBase; + const DWORD protect = rollback ? actions[index].currentProtect : TargetProtection(actions[index]); + size_t runEnd = index + 1; + while(runEnd < count && + actions[runEnd].page == actions[runEnd - 1].page + PAGE_SIZE && + actions[runEnd].allocationBase == allocationBase && + (rollback ? actions[runEnd].currentProtect : TargetProtection(actions[runEnd])) == protect) + { + ++runEnd; + } + + const ptr byteSize = actions[runEnd - 1].page - base + PAGE_SIZE; + if(!process.MemProtect(base, byteSize, protect)) + { + if(appliedCount != nullptr) + *appliedCount = index; + return false; + } + index = runEnd; + } + + if(appliedCount != nullptr) + *appliedCount = count; + return true; } bool Process::SetMemoryBreakpoint(ptr address, ptr size, MemoryType type, bool singleshoot) { + std::lock_guard lock(memoryBreakpointMutex); DPRINTF(); - dprintf("SetMemoryBreakpoint(%p, %p, %d, %d)\n", address, size, type, singleshoot); - //TODO: error reporting - //basic checks - if(!MemIsValidPtr(address) || !size) + // Basic checks, including the range-end overflow that would otherwise wrap + // page enumeration back to address zero. + if(size == 0 || address > ~ptr(0) - (size - 1) || !MemIsValidPtr(address)) return false; - //check if the range is unused for any previous memory breakpoints - auto range = Range(address, address + size - 1); + // Memory breakpoint byte ranges cannot intersect, but disjoint ranges are + // allowed to share their first or last page. + const ptr endAddress = address + size - 1; + const auto range = Range(address, endAddress); if(memoryBreakpointRanges.find(range) != memoryBreakpointRanges.end()) return false; - //change page protections - bool success = true; - struct TempMemoryBreakpointData + // Stage all per-page bookkeeping before changing any protection. A single + // VirtualQueryEx result is reused for every page in that memory region. + const ptr alignedAddress = PAGE_ALIGN(address); + const ptr alignedEnd = PAGE_ALIGN(endAddress); + std::vector actions; + actions.reserve(size_t((alignedEnd - alignedAddress) / PAGE_SIZE + 1)); + + MEMORY_BASIC_INFORMATION mbi = {}; + ptr regionEnd = 0; + for(ptr page = alignedAddress;; page += PAGE_SIZE) { - ptr addr; - DWORD OldProtect; - MemoryBreakpointData data; - }; - std::vector breakpointData; - { - breakpointData.reserve(BYTES_TO_PAGES((address - PAGE_ALIGN(address)) + size)); - TempMemoryBreakpointData tempData; - MemoryBreakpointData data; - data.Type = uint32(type); - auto alignedAddress = PAGE_ALIGN(address); - auto alignedEnd = PAGE_ALIGN(address + size - 1); - for(auto page = alignedAddress; page <= alignedEnd; page += PAGE_SIZE) + if(page >= regionEnd) { - MEMORY_BASIC_INFORMATION mbi; - if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi))) - { - success = false; - dprintf("!VirtualQueryEx\n"); - break; - } - data.OldProtect = mbi.Protect; - if(!SetNewPageProtection(page, data, type)) - { - success = false; - dprintf("!SetNewPageProtection\n"); - break; - } - tempData.addr = page; - tempData.OldProtect = mbi.Protect; - tempData.data = data; - breakpointData.push_back(tempData); + if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi)) || mbi.State != MEM_COMMIT) + return false; + const ptr regionBase = ptr(mbi.BaseAddress); + if(mbi.RegionSize > ~ptr(0) - regionBase) + regionEnd = ~ptr(0); + else + regionEnd = regionBase + ptr(mbi.RegionSize); + if(regionEnd <= page) + return false; } + + MemoryBreakpointPageAction action; + action.page = page; + action.allocationBase = ptr(mbi.AllocationBase); + action.currentProtect = mbi.Protect; + + const auto found = memoryBreakpointPages.find(page); + if(found != memoryBreakpointPages.end()) + action.data = found->second; + else + action.data.OldProtect = mbi.Protect; + + if(!AddMemoryBreakpointReference(action.data, type, permanentDep)) + return false; + actions.push_back(action); + + if(page == alignedEnd) + break; } - //if changing the page protections failed, attempt to revert all protection changes - if(!success) + // Change protections in coalesced runs. If a later run fails, restore every + // earlier run to the actual protection observed while staging. + size_t appliedCount = 0; + if(!ApplyProtectionRuns(*this, actions, actions.size(), false, &appliedCount)) { - for(const auto & page : breakpointData) - MemProtect(page.addr, PAGE_SIZE, page.OldProtect); + ApplyProtectionRuns(*this, actions, appliedCount, true); return false; } - //set the page data - for(const auto & page : breakpointData) - memoryBreakpointPages[page.addr] = page.data; + // Publish page metadata only after the complete protection transaction has + // succeeded. Exception dispatch holds the same recursive mutex and therefore + // cannot observe the debuggee and metadata in different transaction states. + for(const auto & action : actions) + memoryBreakpointPages[action.page] = action.data; - //setup the breakpoint information struct + // Set up and publish the byte-range breakpoint information. BreakpointInfo info = {}; info.address = address; info.singleshoot = singleshoot; info.type = BreakpointType::Memory; info.internal.memory.type = type; info.internal.memory.size = size; - - //insert in the breakpoint map breakpoints.insert({ { info.type, info.address }, info }); memoryBreakpointRanges.insert(range); + dprintf("SetMemoryBreakpoint(%p, %p, %d, %d): %zu pages\n", address, size, type, singleshoot, actions.size()); return true; } bool Process::SetMemoryBreakpoint(ptr address, ptr size, const BreakpointCallback & cbBreakpoint, MemoryType type, bool singleshoot) { + std::lock_guard lock(memoryBreakpointMutex); + //check if a callback on this address was already found if(breakpointCallbacks.find({ BreakpointType::Memory, address }) != breakpointCallbacks.end()) return false; @@ -369,51 +473,85 @@ namespace GleeBug bool Process::DeleteMemoryBreakpoint(ptr address) { - //find the memory breakpoint range - auto range = memoryBreakpointRanges.find(Range(address, address)); + std::lock_guard lock(memoryBreakpointMutex); + + // Find the byte range containing address, then find its breakpoint record. + const auto range = memoryBreakpointRanges.find(Range(address, address)); if(range == memoryBreakpointRanges.end()) return false; - //find the memory breakpoint - auto found = breakpoints.find({ BreakpointType::Memory, range->first }); + const auto found = breakpoints.find({ BreakpointType::Memory, range->first }); if(found == breakpoints.end()) return false; - const auto & info = found->second; + const BreakpointInfo info = found->second; - //delete the memory breakpoint from the pages - bool success = true; - auto alignedAddress = PAGE_ALIGN(info.address); - auto alignedEnd = PAGE_ALIGN(info.address + info.internal.memory.size - 1); - for(auto page = alignedAddress; page <= alignedEnd; page += PAGE_SIZE) + // Stage decremented per-type references and target protections without + // mutating the live page map. Region queries provide both the current + // rollback protection and allocation boundaries for safe coalescing. + const ptr endAddress = info.address + info.internal.memory.size - 1; + const ptr alignedAddress = PAGE_ALIGN(info.address); + const ptr alignedEnd = PAGE_ALIGN(endAddress); + std::vector actions; + actions.reserve(size_t((alignedEnd - alignedAddress) / PAGE_SIZE + 1)); + + MEMORY_BASIC_INFORMATION mbi = {}; + ptr regionEnd = 0; + for(ptr page = alignedAddress;; page += PAGE_SIZE) { - auto foundData = memoryBreakpointPages.find(page); - if(foundData == memoryBreakpointPages.end()) - continue; //TODO: error reporting - auto & data = foundData->second; - DWORD Protect; - data.Refcount--; - if(data.Refcount) + if(page >= regionEnd) { - //TODO: properly determine the new protection flag - //Are there any other protections left? - //If so add the guard - if(data.Type & ~uint32(info.internal.memory.type)) - data.NewProtect = data.OldProtect | PAGE_GUARD; - Protect = data.NewProtect; + if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi)) || mbi.State != MEM_COMMIT) + return false; + const ptr regionBase = ptr(mbi.BaseAddress); + if(mbi.RegionSize > ~ptr(0) - regionBase) + regionEnd = ~ptr(0); + else + regionEnd = regionBase + ptr(mbi.RegionSize); + if(regionEnd <= page) + return false; } - else - Protect = data.OldProtect; - if(!MemProtect(page, PAGE_SIZE, Protect)) - success = false; - if(!data.Refcount) - memoryBreakpointPages.erase(foundData); + + const auto foundData = memoryBreakpointPages.find(page); + if(foundData == memoryBreakpointPages.end()) + return false; + + MemoryBreakpointPageAction action; + action.page = page; + action.allocationBase = ptr(mbi.AllocationBase); + action.currentProtect = mbi.Protect; + action.data = foundData->second; + if(!RemoveMemoryBreakpointReference(action.data, info.internal.memory.type, permanentDep)) + return false; + actions.push_back(action); + + if(page == alignedEnd) + break; } - //delete the breakpoint from the maps + // Keep the live metadata unchanged unless every protection run succeeds. + // Roll back successful runs when a later run fails. + size_t appliedCount = 0; + if(!ApplyProtectionRuns(*this, actions, actions.size(), false, &appliedCount)) + { + ApplyProtectionRuns(*this, actions, appliedCount, true); + return false; + } + + // Commit updated shared-page metadata and release pages whose final + // breakpoint reference was removed. + for(const auto & action : actions) + { + if(action.data.Refcount == 0) + memoryBreakpointPages.erase(action.page); + else + memoryBreakpointPages[action.page] = action.data; + } + + // Delete the byte-range breakpoint only after its page transaction commits. breakpoints.erase(found); - breakpointCallbacks.erase({ BreakpointType::Memory, address }); - memoryBreakpointRanges.erase(Range(address, address)); - return success; + breakpointCallbacks.erase({ BreakpointType::Memory, info.address }); + memoryBreakpointRanges.erase(range); + return true; } bool Process::DeleteGenericBreakpoint(const BreakpointInfo & info) diff --git a/GleeBug/Debugger.Process.h b/GleeBug/Debugger.Process.h index f17d6c1..19520df 100644 --- a/GleeBug/Debugger.Process.h +++ b/GleeBug/Debugger.Process.h @@ -31,6 +31,7 @@ namespace GleeBug BreakpointInfo hardwareBreakpoints[4]; MemoryBreakpointSet memoryBreakpointRanges; MemoryBreakpointMap memoryBreakpointPages; + std::recursive_mutex memoryBreakpointMutex; std::unordered_set recentlyDeletedSwbp; @@ -318,15 +319,6 @@ namespace GleeBug */ bool DeleteHardwareBreakpoint(ptr address); - /** - \brief Sets new page protection to trigger an exception for certain memory breakpoint types. - \param page The page address. - \param data The current protection of the page. - \param type The memory breakpoint type to trigger an exception for. - \return true if it succeeds, false if it fails. - */ - bool SetNewPageProtection(ptr page, MemoryBreakpointData & data, MemoryType type); - /** \brief Sets a memory breakpoint. \param address The address to set the memory breakpoint on. diff --git a/GleeBug/Debugger.cpp b/GleeBug/Debugger.cpp index 1e3b9f5..a987302 100644 --- a/GleeBug/Debugger.cpp +++ b/GleeBug/Debugger.cpp @@ -112,9 +112,76 @@ retry_no_aslr: bool Debugger::UnsafeDetach() { - // TODO: remove from all threads? - Registers(mThread->hThread, CONTEXT_CONTROL).TrapFlag = false; - return !!DebugActiveProcessStop(mMainProcess.dwProcessId); + bool success = true; + + if(mProcess) + { + // 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(); ) + { + auto & info = it->second; + if(info.type == BreakpointType::Software) + { + if(!mProcess->MemWriteUnsafe(info.address, + info.internal.software.oldbytes, + info.internal.software.size)) + success = false; + FlushInstructionCache(mProcess->hProcess, nullptr, 0); + mProcess->softwareBreakpointReferences.erase(info.address); + it = mProcess->breakpoints.erase(it); + } + else + ++it; + } + + // 2. Restore memory breakpoint page protections (drop PAGE_GUARD / + // removed write/execute access) so the debuggee stops faulting on access. + for(auto & page : mProcess->memoryBreakpointPages) + { + DWORD oldProtect = 0; + if(!mProcess->MemProtect(page.first, PAGE_SIZE, + page.second.OldProtect, &oldProtect)) + success = false; + } + mProcess->memoryBreakpointPages.clear(); + mProcess->memoryBreakpointRanges.clear(); + + // 3. Remove hardware breakpoints from every thread, then clear the trap + // flag on every thread (the previous "TODO: remove from all threads"). + // Threads other than the current event's are not guaranteed suspended, + // so suspend/resume them around the context edits to avoid races. + for(auto & kv : mProcess->threads) + { + auto thread = kv.second.get(); + const bool current = (thread == mThread); + if(!current) + SuspendThread(thread->hThread); + + for(int slot = 0; slot < HWBP_COUNT; slot++) + { + if(mProcess->hardwareBreakpoints[slot].internal.hardware.enabled) + thread->DeleteHardwareBreakpoint(HardwareSlot(slot)); + } + + Registers regs(thread->hThread, CONTEXT_CONTROL | CONTEXT_DEBUG_REGISTERS); + regs.TrapFlag = false; + + if(!current) + ResumeThread(thread->hThread); + } + for(int slot = 0; slot < HWBP_COUNT; slot++) + mProcess->hardwareBreakpoints[slot].internal.hardware.enabled = false; + } + else if(mThread) + { + Registers(mThread->hThread, CONTEXT_CONTROL).TrapFlag = false; + } + + if(!DebugActiveProcessStop(mMainProcess.dwProcessId)) + success = false; + + return success; } void Debugger::Detach() diff --git a/GleeBug/GleeBug.h b/GleeBug/GleeBug.h index b4ed193..fe94eac 100644 --- a/GleeBug/GleeBug.h +++ b/GleeBug/GleeBug.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include