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

[Pedro Alves]

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

This is likely not to cause a problem, but it's worth it to consider.

#2

Another thing to consider is the automatic hardware breakpoints
support.  See insert_bp_location.  That means that breakpoint
locations can start as software breakpoints but still end up
inserted as hw breakpoints.  Similarly to #1 above, without the patch,
gdb is considering those locations as duplicates, and after the
patch it no longer will.  So we may end up with multiple insertions
for the same location.  Again, similar to #1.

#3

I suspect this the (automatic hardware breakpoints) can interfere with
e.g., update_breakpoint_locations 's logic of matching old locations
with new locations, in the have_ambiguous_names case.  I.e., after
insertion the locations are hw breakpoint locations, but after
a breakpoint re-set, the new locations will be software breakpoint
locations until they try to be inserted.  Before the patch, the
disabled state will still be carried over.  After the patch, they won't.
I think.

Maybe we need to explicitly consider the case in
update_breakpoint_locations.

Thanks,
Pedro Alves