Merge pull request #79 from x64dbg/membp-bugs

Reimplement setting of memory breakpoints to work better on large ranges

GleeBug/Debugger.Breakpoint.h

@@ -95,10 +95,17 @@ namespace GleeBug
     */
     struct MemoryBreakpointData
     {
-        uint32 Refcount;
+        // Refcount and Type are cached aggregates used by existing page-handling
-        uint32 Type;
+        // code. Per-type counts preserve multiplicity when same-type ranges share
-        DWORD OldProtect;
+        // one page and allow deletion to derive the exact remaining protection.
-        DWORD NewProtect;
+        uint32 Refcount = 0;
+        uint32 Type = 0;
+        uint32 AccessRefs = 0;
+        uint32 ReadRefs = 0;
+        uint32 WriteRefs = 0;
+        uint32 ExecuteRefs = 0;
+        DWORD OldProtect = 0;
+        DWORD NewProtect = 0;
     };
 };
 

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<std::recursive_mutex> 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<std::recursive_mutex> 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
+        // PAGE_GUARD is shared by every breakpoint range on this page. Match against the
-        // ExceptionInformation[0] decides whether this guard-page fault actually belongs to this breakpoint.
+        // range that contains the accessed byte; the aggregate page type may include a
-        // Access breakpoints intentionally match every access type.
+        // 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 breakpointType = info.internal.memory.type;
-        const auto matchesRead = accessType == 0 && (((pageProperties.Type & 0x2) != 0) || isAccessBreakpoint);
+        const auto isAccessBreakpoint = breakpointType == MemoryType::Access;
-        const auto matchesWrite = accessType == 1 && (((pageProperties.Type & 0x4) != 0) || isAccessBreakpoint);
+        const auto matchesRead = accessType == 0 && (breakpointType == MemoryType::Read || isAccessBreakpoint);
-        const auto matchesExecute = accessType == 8 && (((pageProperties.Type & 0x8) != 0) || 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(breakpointCallback)
-        if(bpxCb != mProcess->breakpointCallbacks.end())
+            breakpointCallback(info);
-        {
-            bpxCb->second(info);
-        }
 
 
         mContinueStatus = DBG_CONTINUE;
@@ -351,6 +364,8 @@ namespace GleeBug
 
     void Debugger::exceptionAccessViolation(const EXCEPTION_RECORD & exceptionRecord, bool firstChance)
     {
+        std::unique_lock<std::recursive_mutex> 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.
+        // Access violations generated by memory breakpoints are write or execute faults.
-        //Execute is only implemented with page guard if no Data-Execution-Prevention is implemented by the Kernel.
+        // First verify that the aggregate page state could have caused this fault. A
-        if((exceptionRecord.ExceptionInformation[0] == 1) && (!(pageProperties.Type & 4)))
+        // debuggee-generated violation must still be passed to the debuggee.
-        {
+        const auto accessType = exceptionRecord.ExceptionInformation[0];
-            //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.
+        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(breakpointCallback)
-        if(bpxCb != mProcess->breakpointCallbacks.end())
+            breakpointCallback(info);
-        {
-            bpxCb->second(info);
-        }
 
 
         mContinueStatus = DBG_CONTINUE;

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();
+        if(data.Refcount == 0)
-        //TODO: handle PAGE_NOACCESS and such correctly (since it cannot be combined with PAGE_GUARD)
+            return data.OldProtect;
 
-        auto found = memoryBreakpointPages.find(page);
+        const bool needsGuard = data.AccessRefs != 0 || data.ReadRefs != 0 || (data.ExecuteRefs != 0 && !permanentDep);
-        if(found == memoryBreakpointPages.end())
+        if(needsGuard)
         {
-            data.Refcount = 1;
+            // PAGE_GUARD cannot be combined with PAGE_NOACCESS, PAGE_NOCACHE, or PAGE_WRITECOMBINE.
-            switch(type)
+            if((data.OldProtect & 0xFF) == PAGE_NOACCESS)
-            {
+                return (data.OldProtect & ~0x7FF) | PAGE_NOACCESS;
-            case MemoryType::Access:
+            return (data.OldProtect & ~0x700) | PAGE_GUARD;
-            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;
         }
 
-        dprintf("SetNewPageProtection(%p, %X)\n", page, data.NewProtect);
+        DWORD protect = data.OldProtect;
-        return MemProtect(page, PAGE_SIZE, data.NewProtect);
+        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<MemoryBreakpointPageAction> & 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<std::recursive_mutex> lock(memoryBreakpointMutex);
         DPRINTF();
-        dprintf("SetMemoryBreakpoint(%p, %p, %d, %d)\n", address, size, type, singleshoot);
-        //TODO: error reporting
 
-        //basic checks
+        // Basic checks, including the range-end overflow that would otherwise wrap
-        if(!MemIsValidPtr(address) || !size)
+        // 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
+        // Memory breakpoint byte ranges cannot intersect, but disjoint ranges are
-        auto range = Range(address, address + size - 1);
+        // 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
+        // Stage all per-page bookkeeping before changing any protection. A single
-        bool success = true;
+        // VirtualQueryEx result is reused for every page in that memory region.
-        struct TempMemoryBreakpointData
+        const ptr alignedAddress = PAGE_ALIGN(address);
+        const ptr alignedEnd = PAGE_ALIGN(endAddress);
+        std::vector<MemoryBreakpointPageAction> 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;
+            if(page >= regionEnd)
-            DWORD OldProtect;
-            MemoryBreakpointData data;
-        };
-        std::vector<TempMemoryBreakpointData> 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)
             {
-                MEMORY_BASIC_INFORMATION mbi;
+                if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi)) || mbi.State != MEM_COMMIT)
-                if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi)))
+                    return false;
-                {
+                const ptr regionBase = ptr(mbi.BaseAddress);
-                    success = false;
+                if(mbi.RegionSize > ~ptr(0) - regionBase)
-                    dprintf("!VirtualQueryEx\n");
+                    regionEnd = ~ptr(0);
-                    break;
+                else
-                }
+                    regionEnd = regionBase + ptr(mbi.RegionSize);
-                data.OldProtect = mbi.Protect;
+                if(regionEnd <= page)
-                if(!SetNewPageProtection(page, data, type))
+                    return false;
-                {
-                    success = false;
-                    dprintf("!SetNewPageProtection\n");
-                    break;
-                }
-                tempData.addr = page;
-                tempData.OldProtect = mbi.Protect;
-                tempData.data = data;
-                breakpointData.push_back(tempData);
             }
+
+            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
+        // Change protections in coalesced runs. If a later run fails, restore every
-        if(!success)
+        // 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)
+            ApplyProtectionRuns(*this, actions, appliedCount, true);
-                MemProtect(page.addr, PAGE_SIZE, page.OldProtect);
             return false;
         }
 
-        //set the page data
+        // Publish page metadata only after the complete protection transaction has
-        for(const auto & page : breakpointData)
+        // succeeded. Exception dispatch holds the same recursive mutex and therefore
-            memoryBreakpointPages[page.addr] = page.data;
+        // 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<std::recursive_mutex> 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
+        std::lock_guard<std::recursive_mutex> lock(memoryBreakpointMutex);
-        auto range = memoryBreakpointRanges.find(Range(address, address));
+
+        // 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
+        const auto found = breakpoints.find({ BreakpointType::Memory, range->first });
-        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
+        // Stage decremented per-type references and target protections without
-        bool success = true;
+        // mutating the live page map. Region queries provide both the current
-        auto alignedAddress = PAGE_ALIGN(info.address);
+        // rollback protection and allocation boundaries for safe coalescing.
-        auto alignedEnd = PAGE_ALIGN(info.address + info.internal.memory.size - 1);
+        const ptr endAddress = info.address + info.internal.memory.size - 1;
-        for(auto page = alignedAddress; page <= alignedEnd; page += PAGE_SIZE)
+        const ptr alignedAddress = PAGE_ALIGN(info.address);
+        const ptr alignedEnd = PAGE_ALIGN(endAddress);
+        std::vector<MemoryBreakpointPageAction> 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(page >= regionEnd)
-            if(foundData == memoryBreakpointPages.end())
-                continue; //TODO: error reporting
-            auto & data = foundData->second;
-            DWORD Protect;
-            data.Refcount--;
-            if(data.Refcount)
             {
-                //TODO: properly determine the new protection flag
+                if(!VirtualQueryEx(hProcess, LPCVOID(page), &mbi, sizeof(mbi)) || mbi.State != MEM_COMMIT)
-                //Are there any other protections left?
+                    return false;
-                //If so add the guard
+                const ptr regionBase = ptr(mbi.BaseAddress);
-                if(data.Type & ~uint32(info.internal.memory.type))
+                if(mbi.RegionSize > ~ptr(0) - regionBase)
-                    data.NewProtect = data.OldProtect | PAGE_GUARD;
+                    regionEnd = ~ptr(0);
-                Protect = data.NewProtect;
+                else
+                    regionEnd = regionBase + ptr(mbi.RegionSize);
+                if(regionEnd <= page)
+                    return false;
             }
-            else
+
-                Protect = data.OldProtect;
+            const auto foundData = memoryBreakpointPages.find(page);
-            if(!MemProtect(page, PAGE_SIZE, Protect))
+            if(foundData == memoryBreakpointPages.end())
-                success = false;
+                return false;
-            if(!data.Refcount)
+
-                memoryBreakpointPages.erase(foundData);
+            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 });
+        breakpointCallbacks.erase({ BreakpointType::Memory, info.address });
-        memoryBreakpointRanges.erase(Range(address, address));
+        memoryBreakpointRanges.erase(range);
-        return success;
+        return true;
     }
 
     bool Process::DeleteGenericBreakpoint(const BreakpointInfo & info)

GleeBug/Debugger.Process.h

@@ -31,6 +31,7 @@ namespace GleeBug
         BreakpointInfo hardwareBreakpoints[4];
         MemoryBreakpointSet memoryBreakpointRanges;
         MemoryBreakpointMap memoryBreakpointPages;
+        std::recursive_mutex memoryBreakpointMutex;
 
         std::unordered_set<ptr> 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.

GleeBug/Debugger.cpp

@@ -112,9 +112,76 @@ retry_no_aslr:
 
     bool Debugger::UnsafeDetach()
     {
-        // TODO: remove from all threads?
+        bool success = true;
-        Registers(mThread->hThread, CONTEXT_CONTROL).TrapFlag = false;
+
-        return !!DebugActiveProcessStop(mMainProcess.dwProcessId);
+        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()

GleeBug/GleeBug.h

@@ -10,6 +10,7 @@
 #include <set>
 #include <functional>
 #include <algorithm>
+#include <mutex>
 
 #include <windows.h>
 #include <psapi.h>