[RFA] gdb/13333: remote crash

[Keith Seitz <keiths at redhat dot com>]

Hi,

I've been looking at this bug, which causes a SEGV in gdb when the user 
steps past an inferior exit (and the native gdbserver exits). Here's a 
transcript which demonstrates the problem:

$ ./gdb -nx -q ~/tmp/13333
Reading symbols from /home/keiths/tmp/13333...done.
(gdb) b main
Breakpoint 1 at 0x400478: file 13333.c, line 6.
(gdb) tar remote :1234
Remote debugging using :1234
Reading symbols from /lib64/ld-linux-x86-64.so.2...(no debugging symbols 
found)...done.
Loaded symbols for /lib64/ld-linux-x86-64.so.2
0x0000003d962016b0 in _start () from /lib64/ld-linux-x86-64.so.2
(gdb) c
Continuing.

Breakpoint 1, main () at 13333.c:6
6	 return 0;
(gdb) n
7	}
(gdb)
0x0000003d9662139d in __libc_start_main () from /lib64/libc.so.6
(gdb)
Single stepping until exit from function __libc_start_main,
which has no line number information.
[Inferior 1 (Remote target) exited normally]
Segmentation fault (core dumped)

The segfault occurs because remote_remove_breakpoint is attempting to 
call putpkt when remote_desc is NULL (because target_close has already 
closed the serial channel and set remote_desc to NULL).

I've spent a fair amount of time attempting to track exactly when this 
was broken, and whether it was a logic error somewhere, and I *think* 
I've just about convinced myself that this is simply an oversight.

This bug was inadvertently introduced in commit 625c318c:

2010-12-09  Tom Tromey  <tromey@redhat.com>

        PR c++/9593:
        * thread.c (clear_thread_inferior_resources): Call
        delete_longjmp_breakpoint.
        * infrun.c (handle_inferior_event): Handle exception breakpoints.
        (handle_inferior_event): Likewise.
        (insert_exception_resume_breakpoint): New function.
        (check_exception_resume): Likewise.
        * inferior.h (delete_longjmp_breakpoint_cleanup): Declare.
        * infcmd.c (delete_longjmp_breakpoint_cleanup): No longer static.
[snip]

The important part here is that a call to delete_longjmp_breakpoint was 
moved from a cleanup in handle_inferior_event to 
clear_thread_inferior_resources. Allow me to elaborate.

Previously when the inferior exited, target_mourn_inferior would call 
unpush_target, which would call remote_close (setting remote_desc to 
NULL), and return (after doing some other stuff). This would then pop 
the remote target, leaving the "exec" target at the top of the target stack.

Back in handle_inferior_event, the delete_longjmp_cleanup would be run, 
and calling delete_longjmp_breakpoint, which calls 
target_remove_breakpoint. Because the exec target is currently the top 
of the stack, to_remove_breakpoint is set to "ignore", and this function 
does what it needs to do. Everything is good.

After the patch, though, the subtle change is the call to 
delete_longjmp_breakpoint was pushed down under unpush_target. As a 
result, when target_remove_breakpoint is called (from 
delete_longjmp_breakpoint in clear_thread_inferior_resources), it ends 
up calling remote_remove_breakpoint, which doesn't check whether 
remote_desc is valid or not, and POOF! We have the crash.

I believe that the right thing to do is keep the code paths identical in 
both of these cases, and that is achieved by simply checking remote_desc 
in remote_remove_breakpoint.

Having done that, the test suite shows no regressions, and the internal 
breakpoint list on both the working and patched trees give identical 
results.

So, remote.c wizards -- what say ye?

Keith

ChangeLog
2011-11-08  Keith Seitz  <keiths@redhat.com>

	PR gdb/13333
	* remote.c (remote_remove_breakpoint): Do not attempt to
	push packets down the wire if REMOTE_DESC is NULL.

testsuite/ChangeLog
2011-11-08  Keith Seitz  <keiths@redhat.com>

	PR gdb/13333
	* gdb.server/server-run.exp: Check if gdb crashes when
	the inferior and gdbserver exit while stepping.

diff --git a/gdb/remote.c b/gdb/remote.c
index 5182ef1..63c99cf 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7703,28 +7703,33 @@ static int
 remote_remove_breakpoint (struct gdbarch *gdbarch,
 			  struct bp_target_info *bp_tgt)
 {
-  CORE_ADDR addr = bp_tgt->placed_address;
-  struct remote_state *rs = get_remote_state ();
-
-  if (remote_protocol_packets[PACKET_Z0].support != PACKET_DISABLE)
+  if (remote_desc != NULL)
     {
-      char *p = rs->buf;
+      CORE_ADDR addr = bp_tgt->placed_address;
+      struct remote_state *rs = get_remote_state ();
 
-      *(p++) = 'z';
-      *(p++) = '0';
-      *(p++) = ',';
+      if (remote_protocol_packets[PACKET_Z0].support != PACKET_DISABLE)
+	{
+	  char *p = rs->buf;
 
-      addr = (ULONGEST) remote_address_masked (bp_tgt->placed_address);
-      p += hexnumstr (p, addr);
-      sprintf (p, ",%d", bp_tgt->placed_size);
+	  *(p++) = 'z';
+	  *(p++) = '0';
+	  *(p++) = ',';
 
-      putpkt (rs->buf);
-      getpkt (&rs->buf, &rs->buf_size, 0);
+	  addr = (ULONGEST) remote_address_masked (bp_tgt->placed_address);
+	  p += hexnumstr (p, addr);
+	  sprintf (p, ",%d", bp_tgt->placed_size);
 
-      return (rs->buf[0] == 'E');
+	  putpkt (rs->buf);
+	  getpkt (&rs->buf, &rs->buf_size, 0);
+
+	  return (rs->buf[0] == 'E');
+	}
+
+      return memory_remove_breakpoint (gdbarch, bp_tgt);
     }
 
-  return memory_remove_breakpoint (gdbarch, bp_tgt);
+  return 1;
 }
 
 static int
diff --git a/gdb/testsuite/gdb.server/server-run.exp b/gdb/testsuite/gdb.server/server-run.exp
index 9567043..6cd38f8 100644
--- a/gdb/testsuite/gdb.server/server-run.exp
+++ b/gdb/testsuite/gdb.server/server-run.exp
@@ -47,3 +47,20 @@ if { [istarget *-*-linux*] } {
 
 gdb_breakpoint main
 gdb_test "continue" "Breakpoint.* main .*" "continue to main"
+
+# gdb/13333 - Check if gdb crashes while stepping over an inferior exit.
+send_gdb "next\n"
+gdb_expect {
+  -re "exited normally.*$gdb_prompt $" {
+    # The inferior exited, and gdb did not crash.
+    pass "step until inferior exits"
+  }
+
+  -re "$gdb_prompt $" {
+    # We got a prompt -- keep stepping
+    send_gdb "next\n"
+    exp_continue
+  }
+
+  default { fail "step until inferior exits" }
+}