Re: [PATCH] breakpoint: Make sure location types match before swapping

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Pedro Alves <palves@redhat.com> [2020-04-14 20:17:00 +0100]:

> On 4/14/20 5:00 PM, Andrew Burgess wrote:
> 
> > The other slight issue I had with the patch was that based on the
> > original description I still had to go and figure out the exact
> > conditions that would trigger this bug.  I believe that to trigger
> > this you need a h/w breakpoint placed on an instruction that loops to
> > itself, I don't see how else this could happen.
> > 
> > I took a crack at writing a more detailed break down of the conditions
> > that trigger this bug.
> > 
> 
> > I'm still running this through testing here, but I'd be interested to
> > hear if you think the fuller description is helpful.
> 
> It is.
> 
> > From d2b719b0f4857461064ed7b1da744a01b2ad2c6d Mon Sep 17 00:00:00 2001
> > From: Andrew Burgess <andrew.burgess@embecosm.com>
> > Date: Tue, 14 Apr 2020 16:32:02 +0100
> > Subject: [PATCH] gdb: ensure location types match before swapping locations
> > 
> > In the following conditions:
> > 
> >   - A target with hardware breakpoints available, and
> >   - A target that uses software single stepping,
> >   - An instruction at ADDRESS loops back to itself,
> > 
> > Now consider the following steps:
> > 
> >   1. The user places a hardware breakpoint at ADDRESS (an instruction
> >   that loops to itself),
> > 
> >   2. The inferior runs and hits the breakpoint from #1,
> > 
> >   3. The user tells GDB to 'continue'.
> > 
> > In #3 when the user tells GDB to continue, GDB first disables the
> > hardware breakpoint at ADDRESS, and then inserts a software single
> > step breakpoint at ADDRESS.  The original user created breakpoint was
> > a hardware breakpoint, while the single step breakpoint will be a
> > software breakpoint.
> > 
> > GDB continues and immediately hits the software single step
> > breakpoint.
> > 
> > GDB then deletes the software single step breakpoint by calling
> > delete_single_step_breakpoints, which eventually calls
> > delete_breakpoint, which, once the breakpoint (and its locations) are
> > deleted, calls update_global_location_list.
> > 
> > During update_global_location_list GDB spots that we have an old
> > location (the software single step breakpoint location) that is
> > inserted, but being deleted, and a location at the same address which
> > we are keeping, but which is not currently inserted (the original
> > hardware breakpoint location), GDB then calls
> > breakpoint_locations_match on these two locations.  Currently the
> > locations do match, and so GDB calls swap_insertion which swaps the
> > "inserted" state of the two locations.  GDB finally returns through
> > the call stack and leaves delete_single_step_breakpoints.  After this
> > GDB continues with its normal "stopping" process, as part of this
> > stopping process GDB removes all the breakpoints from the target.  Due
> > to the swap the original hardware breakpoint location is removed.
> > 
> > The problem is that GDB inserted the software single step breakpoint
> > as a software breakpoint, and then, thanks to the swap, tries to
> > remove it as a hardware breakpoint.  This will leave the target in an
> > inconsistent state, and as in the original bug report (PR gdb/25741),
> > could result in the target throwing an error.
> > 
> > The solution for all this is to have two breakpoint locations of
> > different types (hardware breakpoint vs software breakpoint) not
> > match.  The result of this is that GDB will no longer swap the
> > inserted status of the two breakpoints.  The original call to
> > update_global_location_list (after deleting the software single step
> > breakpoint) will at this point trigger a call to remove the
> > breakpoint, something which will be done based on the type of that
> > location.  Later GDB will see that the original hardware breakpoint is
> > already not inserted, and so will leave it alone.
> > 
> > This patch was original proposed by Keno Fischer here:
> > 
> >   https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
> > 
> > gdb/ChangeLog:
> > 
> > 	PR gdb/25741
> > 	* breakpoint.c (breakpoint_locations_match): Compare location
> > 	types.
> 
> This seems right at first blush, though there are some things
> that we need to look at.  Here are 3 cases that found while
> looking for breakpoint_locations_match callers:
> 
> #1
> 
> E.g., with this, GDB will now install both a hw breakpoint location
> and a software location at the same address.  E.g., a contrived case
> to see it happen would be, with:
> 
>  (gdb) set breakpoint always-inserted on
>  (gdb) set debug remote 1
> 
> before:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) b main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
> 
> after:
> 
>  (gdb) hbreak main
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Hardware assisted breakpoint 1 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  (gdb) break main
>  Note: breakpoint 1 also set at pc 0x400736.
>  Sending packet: $m400736,1#fe...Packet received: 48
>  Breakpoint 2 at 0x400736: file threads.c, line 57.
>  Sending packet: $Z1,400736,1#48...Packet received: OK
>  Sending packet: $Z0,400736,1#47...Packet received: OK

While working on a revised patch I tried to reproduce this, and it's
most odd.  Notice that both before and after you have two Z1 packets,
you're inserting the hardware breakpoint twice with no intermediate
removal.

I don't see this behaviour, even on an unmodified GDB, and I know some
remove targets, for example OpenOCD, will sanity check against such
double insertions and throw an error.

Just though this was worth mentioning.

Anyway, here's a revised patch.  I've moved the location of the type
check so it is only done now for the swapping case.  This should
resolve all the concerns you raised, while still addressing the
original issue.  I updated the commit message a bit too.

What do you think?

Thanks,
Andrew

---

commit 10d62976088600ee39b9b828bae93f82a44a2974
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Tue Apr 14 16:32:02 2020 +0100

    gdb: ensure location types match before swapping locations
    
    In the following conditions:
    
      - A target with hardware breakpoints available, and
      - A target that uses software single stepping,
      - An instruction at ADDRESS loops back to itself,
    
    Now consider the following steps:
    
      1. The user places a hardware breakpoint at ADDRESS (an instruction
      that loops to itself),
    
      2. The inferior runs and hits the breakpoint at ADDRESS,
    
      3. The user tells GDB to 'continue'.
    
    In #3 when the user tells GDB to continue, GDB first disables the
    hardware breakpoint at ADDRESS, and then inserts a software single
    step breakpoint at ADDRESS.  The original user created breakpoint was
    a hardware breakpoint, while the single step breakpoint will be a
    software breakpoint.
    
    GDB continues and immediately hits the software single step
    breakpoint.
    
    GDB then deletes the software single step breakpoint by calling
    delete_single_step_breakpoints, which eventually calls
    delete_breakpoint, which, once the breakpoint (and its locations) are
    deleted, calls update_global_location_list.
    
    During update_global_location_list GDB spots that we have an old
    location (the software single step breakpoint location) that is
    inserted, but being deleted, and a location (the original hardware
    breakpoint) at the same address which we are keeping, but which is not
    currently inserted, GDB then calls breakpoint_locations_match on these
    two locations.
    
    Currently the locations do match, and so GDB calls swap_insertion
    which swaps the "inserted" state of the two locations.  The user
    created hardware breakpoint is marked as inserted, while the GDB
    internal software single step breakpoint is now marked as not
    inserted.  After this GDB returns through the call stack and leaves
    delete_single_step_breakpoints.
    
    After this GDB continues with its normal "stopping" process, as part
    of this stopping process GDB removes all the breakpoints from the
    target.  Due to the swap it is now the user created hardware
    breakpoint that is marked as inserted, so it is this breakpoint GDB
    tries to remove.
    
    The problem is that GDB inserted the software single step breakpoint
    as a software breakpoint, but is now trying to remove the hardware
    breakpoint.  The problem is removing a software breakpoint is very
    different to removing a hardware breakpoint, this could result is some
    undetected undefined behaviour, or as in the original bug report (PR
    gdb/25741), could result in the target throwing an error.
    
    This original patch proposed by Keno Fischer is here:
    
      https://sourceware.org/pipermail/gdb-patches/2020-April/167202.html
    
    This patch modified breakpoint_locations_match so that two breakpoints
    of different types would no longer match.  This resolves the immediate
    problem described above, but introduces some other issues.
    
    The breakpoint_locations_match function is also used to control
    whether two breakpoints places at the same address should both be
    inserted or not.
    
    Consider this (slightly contrived) use case:
    
      (gdb) set debug remote 1
      (gdb) set breakpoint  always-inserted on
      (gdb) hbreak  MyFunc0
      Sending packet: $me0000202,2#84...Packet received: 0000
      Hardware assisted breakpoint 3 at 0xe0000202: file test.c, line 288.
      Sending packet: $m80000188,1#63...Packet received: 00
      Sending packet: $Z1,e0000202,2#ce...Packet received: OK
      (gdb) break MyFunc0
      Note: breakpoint 3 also set at pc 0xe0000202.
      Sending packet: $me0000202,2#84...Packet received: 0000
      Breakpoint 4 at 0xe0000202: file test.c, line 288.
      Sending packet: $m80000188,1#63...Packet received: 00
    
    Notice after setting the hardware breakpoint we see the Z1 packet
    sent, but after setting the software breakpoint there's no Z0 packet,
    GDB sees that the locations match and doesn't make the second
    insertion.  If we modify breakpoint_locations_match then we would
    insert both a hardware _and_ software breakpoint.  This probably
    doesn't matter, but is not ideal, so a solution that doesn't do this
    would be better.
    
    The proposal here is to focus specifically on the case where we are
    considering swapping the inserted status of two breakpoint locations,
    and moves the location type check out of breakpoint_locations_match,
    and into the caller.  This resolves the original issue, while avoiding
    the double insertion problem.
    
    gdb/ChangeLog:
    
            PR gdb/25741
            * breakpoint.c (breakpoint_locations_match): Compare location
            types.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index e49025461ba..afd6459a634 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -11786,6 +11786,17 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
 			      gdb_assert (is_hardware_watchpoint (loc2->owner));
 			      loc2->watchpoint_type = old_loc->watchpoint_type;
 			    }
+			  else if (is_breakpoint (old_loc->owner))
+			    {
+			      /* If we swap the inserted status of a
+				 hardware and software breakpoint then GDB
+				 will insert the breakpoint as one type,
+				 and later try to remove the breakpoint as
+				 the other type.  This is not good.  */
+			      gdb_assert (is_breakpoint (loc2->owner));
+			      if (old_loc->loc_type != loc2->loc_type)
+				continue;
+			    }
 
 			  /* loc2 is a duplicated location. We need to check
 			     if it should be inserted in case it will be