LibDebug: Make sure to not single step the program twice

After hitting a breakpoint, we single step the program to execute the
instruction we breaked on and re-enable the breakpoint.
We also single step the program when the user of LibDebug returned a
DebugDecision::SingleStep.

Previously, if we hit a breakpoint and then were asked to to a
DebugDecision::SingleStep, we would single step twice.

This bug can actually crash programs, because it might cause us to
skip over a patched INT3 instruction in the second single-step.

Interestingely enough, this bug manifested as functrace crashing
certain programs: after hitting a breakpoint on a CALL instruction,
functrace single steps the program to see where the CALL jumps to
(yes, this can be optimized :D). functrace crashed when a CALL
instruction jumps to another CALL, because it inserts breakpoints on CALL
instructions, and so the INT3 in the 2nd CALL was skipped over, and we
executed garbage :).

This commit fixes this by making sure not to single-step twice.

2 files changed, 22 insertions, 2 deletions

diff --git a/Libraries/LibDebug/DebugSession.cpp b/Libraries/LibDebug/DebugSession.cpp
index 91c6f38dcc..97d1d8d8f4 100644
--- a/Libraries/LibDebug/DebugSession.cpp
+++ b/Libraries/LibDebug/DebugSession.cpp
@@ -225,6 +225,12 @@ int DebugSession::continue_debugee_and_wait(ContinueType type)
 
 void* DebugSession::single_step()
 {
+    // Single stepping works by setting the x86 TRAP flag bit in the eflags register.
+    // This flag causes the cpu to enter single-stepping mode, which causes
+    // Interupt 1 (debug interrupt) to be emitted after every instruction.
+    // To single step the program, we set the TRAP flag and continue the debugee.
+    // After the debugee has stopped, we clear the TRAP flag.
+
     auto regs = get_registers();
     constexpr u32 TRAP_FLAG = 0x100;
     regs.eflags |= TRAP_FLAG;
diff --git a/Libraries/LibDebug/DebugSession.h b/Libraries/LibDebug/DebugSession.h
index 20a549f777..5522298383 100644
--- a/Libraries/LibDebug/DebugSession.h
+++ b/Libraries/LibDebug/DebugSession.h
@@ -173,13 +173,20 @@ void DebugSession::run(Callback callback)
         }
 
         if (current_breakpoint.has_value()) {
-            // We want to make the breakpoint transparrent to the user of the debugger
+            // We want to make the breakpoint transparrent to the user of the debugger.
+            // To achieive this, we perform two rollbacks:
+            // 1. Set regs.eip to point at the actual address of the instruction we breaked on.
+            //    regs.eip currently points to one byte after the address of the original instruction,
+            //    because the cpu has just executed the INT3 we patched into the instruction.
+            // 2. We restore the original first byte of the instruction,
+            //    because it was patched with INT3.
             regs.eip = reinterpret_cast<u32>(current_breakpoint.value().address);
             set_registers(regs);
             disable_breakpoint(current_breakpoint.value().address);
         }
 
         DebugBreakReason reason = (state == State::Syscall && !current_breakpoint.has_value()) ? DebugBreakReason::Syscall : DebugBreakReason::Breakpoint;
+
         DebugDecision decision = callback(reason, regs);
 
         if (reason == DebugBreakReason::Syscall) {
@@ -194,10 +201,17 @@ void DebugSession::run(Callback callback)
             state = State::Syscall;
         }
 
+        bool did_single_step = false;
+
         // Re-enable the breakpoint if it wasn't removed by the user
         if (current_breakpoint.has_value() && m_breakpoints.contains(current_breakpoint.value().address)) {
+            // The current breakpoint was removed in order to make it transparrent to the user.
+            // We now want to re-enable it - the code execution flow could hit it again.
+            // To re-enable the breakpoint, we first perform a single step and execute the
+            // instruction of the breakpoint, and then redo the INT3 patch in its first byte.
             auto stopped_address = single_step();
             enable_breakpoint(current_breakpoint.value().address);
+            did_single_step = true;
             // If there is another breakpoint after the current one,
             // Then we are already on it (because of single_step)
             auto breakpoint_at_next_instruction = m_breakpoints.get(stopped_address);
@@ -215,7 +229,7 @@ void DebugSession::run(Callback callback)
             ASSERT_NOT_REACHED(); // TODO: implement
         }
 
-        if (state == State::SingleStep) {
+        if (state == State::SingleStep && !did_single_step) {
             single_step();
         }
     }