From f9d62fd5e56df51053934ec77e8975f8eef91b97 Mon Sep 17 00:00:00 2001 From: Itamar Date: Sat, 23 May 2020 17:11:11 +0300 Subject: [PATCH] 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. --- Libraries/LibDebug/DebugSession.cpp | 6 ++++++ Libraries/LibDebug/DebugSession.h | 18 ++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Libraries/LibDebug/DebugSession.cpp b/Libraries/LibDebug/DebugSession.cpp index 91c6f38dccb..97d1d8d8f4a 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 20a549f7771..5522298383c 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(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(); } }