Merge pull request #76 from 3rdit/fix/swbp-improvements

Fix software breakpoint handling issues

GleeBug/Debugger.Loop.Exception.cpp

@@ -6,7 +6,8 @@ namespace GleeBug
     void Debugger::exceptionBreakpoint(const EXCEPTION_RECORD & exceptionRecord, const bool firstChance)
     {
         //check if the breakpoint exists
-        auto foundInfo = mProcess->breakpoints.find({ BreakpointType::Software, ptr(exceptionRecord.ExceptionAddress) });
+        auto exceptionAddress = ptr(exceptionRecord.ExceptionAddress);
+        auto foundInfo = mProcess->breakpoints.find({ BreakpointType::Software, exceptionAddress });
         if(foundInfo == mProcess->breakpoints.end())
         {
             if(!this->mAttachedToProcess && !mProcess->systemBreakpoint) //handle system breakpoint
@@ -18,6 +19,17 @@ namespace GleeBug
                 //call the callback
                 cbSystemBreakpoint();
             }
+            else
+            {
+                //check if this address had a breakpoint that was recently deleted
+                auto& deletedBps = mProcess->recentlyDeletedSwbp;
+                auto foundIt = deletedBps.find(exceptionAddress);
+                if(foundIt != deletedBps.end() && mThread)
+                {
+                    Registers(mThread->hThread, CONTEXT_CONTROL).Gip = exceptionAddress;
+                    mContinueStatus = DBG_CONTINUE;
+                }
+            }
             return;
         }
 
@@ -30,12 +42,26 @@ namespace GleeBug
         Registers(mThread->hThread, CONTEXT_CONTROL).Gip = info.address;
 
         //restore the original breakpoint byte and do an internal step
-        mProcess->MemWriteUnsafe(info.address, info.internal.software.oldbytes, info.internal.software.size);
+        if(!mProcess->MemWriteUnsafe(info.address, info.internal.software.oldbytes, info.internal.software.size))
+        {
+            //failed to restore original byte, pass exception to debuggee
+            mContinueStatus = DBG_EXCEPTION_NOT_HANDLED;
+            return;
+        }
         mProcess->StepInternal([this, info]()
         {
             //only restore the bytes if the breakpoint still exists
-            if(mProcess->breakpoints.find({ BreakpointType::Software, info.address }) != mProcess->breakpoints.end())
-                mProcess->MemWriteUnsafe(info.address, info.internal.software.newbytes, info.internal.software.size);
+            auto foundBreakpoint = mProcess->breakpoints.find({ BreakpointType::Software, info.address });
+            if(foundBreakpoint != mProcess->breakpoints.end())
+            {
+                if(!mProcess->MemWriteUnsafe(info.address, info.internal.software.newbytes, info.internal.software.size))
+                {
+                    //failed to restore breakpoint byte, remove from maps to stay consistent
+                    mProcess->softwareBreakpointReferences.erase(info.address);
+                    mProcess->breakpoints.erase(foundBreakpoint);
+                    mProcess->breakpointCallbacks.erase({ BreakpointType::Software, info.address });
+                }
+            }
         });
 
         //call the generic callback

GleeBug/Debugger.Loop.cpp

@@ -40,6 +40,8 @@ namespace GleeBug
             IsDbgReplyLaterSupported = mSafeStep;
         }
 
+        uint32 consecutiveTimeouts = 0;
+
         while(!mBreakDebugger)
         {
             //wait for a debug event
@@ -65,11 +67,18 @@ namespace GleeBug
 #endif
                 else
                 {
-                    // Regular timeout, wait again
+                    //after 2 consecutive timeouts, clear recently deleted breakpoints
+                    //any stale events would have been delivered by now
+                    consecutiveTimeouts++;
+                    if(consecutiveTimeouts >= 2 && mProcess)
+                        mProcess->recentlyDeletedSwbp.clear();
                     continue;
                 }
             }
 
+            //event received, reset timeout counter
+            consecutiveTimeouts = 0;
+
             // Handle safe stepping
             if(IsDbgReplyLaterSupported)
             {

GleeBug/Debugger.Process.Breakpoint.cpp

@@ -68,6 +68,8 @@ namespace GleeBug
             return false;
         FlushInstructionCache(hProcess, nullptr, 0);
 
+        recentlyDeletedSwbp.insert(address);
+
         //remove the breakpoint from the maps
         softwareBreakpointReferences.erase(info.address);
         breakpoints.erase(found);

GleeBug/Debugger.Process.Memory.cpp

@@ -65,8 +65,58 @@ namespace GleeBug
 
     bool Process::MemWriteSafe(ptr address, const void* buffer, ptr size, ptr* bytesWritten)
     {
-        //TODO: correctly implement this
-        return MemWrite(address, buffer, size, bytesWritten, false);
+        if(size == 0)
+        {
+            if(bytesWritten)
+                *bytesWritten = 0;
+            return true;
+        }
+
+        std::vector<uint8> copy((const uint8*)buffer, (const uint8*)buffer + size);
+
+        auto start = address;
+        auto end = start + size;
+
+        //find overlapping software breakpoints and preserve their 0xCC bytes in the copy (so write doesn't remove breakpoints)
+        //as well as track what oldbytes values need updating after successful write
+        std::vector<std::tuple<uint8*, uint8, ptr>> pendingUpdates; //tuple: (pointer to oldbyte, new value, offset from start for partial write handling)
+
+        for(auto & breakpoint : breakpoints)
+        {
+            if(breakpoint.first.first != BreakpointType::Software)
+                continue;
+            auto & info = breakpoint.second;
+            auto curAddress = info.address;
+            for(ptr j = 0; j < info.internal.software.size; j++)
+            {
+                if(curAddress + j >= start && curAddress + j < end)
+                {
+                    auto offset = curAddress + j - start;
+                    pendingUpdates.emplace_back(&info.internal.software.oldbytes[j], copy[offset], offset);
+                    copy[offset] = info.internal.software.newbytes[j];
+                }
+            }
+        }
+
+        //write to memory (breakpoint bytes are preserved in the copy)
+        ptr written = 0;
+        if(!MemWriteUnsafe(address, copy.data(), size, &written))
+        {
+            if(bytesWritten)
+                *bytesWritten = written;
+            return false;
+        }
+
+        //apply oldbytes updates only for bytes that were actually written
+        for(const auto & update : pendingUpdates)
+        {
+            if(std::get<2>(update) < written)
+                *std::get<0>(update) = std::get<1>(update);
+        }
+
+        if(bytesWritten)
+            *bytesWritten = written;
+        return true;
     }
 
     bool Process::MemIsValidPtr(ptr address) const

GleeBug/Debugger.Process.h

@@ -32,6 +32,8 @@ namespace GleeBug
         MemoryBreakpointSet memoryBreakpointRanges;
         MemoryBreakpointMap memoryBreakpointPages;
 
+        std::unordered_set<ptr> recentlyDeletedSwbp;
+
         /**
         \brief Constructor.
         \param hProcess Process handle.