From ac95555111931d77b46ca6ad9fff51ae7db61be9 Mon Sep 17 00:00:00 2001
From: Malfurious <m@lfurio.us>
Date: Sat, 27 Apr 2024 12:53:38 -0400
Subject: Fix removal of temporary breakpoints when rapidly restarting thread

This bug would occur when the debugger keeps rapidly hitting a
breakpoint and restarting execution (and the breakpoint is temporary /
one-time).  For example, stepping over a long recursive call.

It would be possible for the user to manually interrupt execution at the
precise moment that the debugee was returning from a single step over
the breakpoint.  In this scenario, the thread would correctly remain
stopped as requested, however the temporary breakpoint would incorrectly
remain in the breakpoint list.  The cause of this issue is that
uninstall_breakpoints only considered removing temporaries if they were
currently installed for use.

This makes sense, as we have used the b->installed flag as a sort of
check to see if we have YET installed the breakpoint (rather than "is it
STILL installed?") to ensure that we don't remove it too soon.

The more accurate logic here is to acctually check whether the
breakpoint has EVER been installed and only then consider it for removal
if it is temporary.  This fixes the case in question without breaking
behavior when performing an initial run single step.

Signed-off-by: Malfurious <m@lfurio.us>
---
 debugger.c | 19 ++++++++++---------
 debugger.h |  1 +
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/debugger.c b/debugger.c
index bacd09c..fdff032 100644
--- a/debugger.c
+++ b/debugger.c
@@ -79,6 +79,7 @@ static void install_breakpoints(struct thread *th) {
             word = (word & ~0xff) | BREAKPOINT_INSN;
             ptrace(PTRACE_POKETEXT, th->id, b->address, word);
             b->installed = 1;
+            b->previously_installed = 1;
         }
     }
 }
@@ -89,16 +90,15 @@ static void uninstall_breakpoints(struct thread *th) {
         if (b->installed) {
             ptrace(PTRACE_POKETEXT, th->id, b->address, b->text);
             b->installed = 0;
+        }
 
-            if (b->enabled < 0) {
-                struct thread *t;
-                if (b->tid == 0 ||
-                        ((t = thread_by_id(th->proc, b->tid)) && !t->doing)) {
-                    struct breakpoint *del = b;
-                    b = b->next;
-                    list_remove(del);
-                    free(del);
-                }
+        if (b->previously_installed && b->enabled < 0) {
+            struct thread *t = NULL;
+            if (b->tid == 0 || ((t = thread_by_id(th->proc, b->tid)) && !t->doing)) {
+                struct breakpoint *del = b;
+                b = b->next;
+                list_remove(del);
+                free(del);
             }
         }
     }
@@ -434,6 +434,7 @@ struct breakpoint *add_breakpoint(struct process *proc, unsigned long address) {
     b->address = address;
     b->text = 0;
     b->installed = 0;
+    b->previously_installed = 0;
     b->hits = 0;
     b->user = 1;
     b->stack = 0;
diff --git a/debugger.h b/debugger.h
index cea3bba..f14459f 100644
--- a/debugger.h
+++ b/debugger.h
@@ -11,6 +11,7 @@ struct breakpoint {
     unsigned long address;
     unsigned long text;
     int installed;
+    int previously_installed;
     int hits;
     int user;
 
-- 
cgit v1.2.3