[PATCH] remote: Fix a crash on longjmp breakpoint removal

[PATCH] remote: Fix a crash on longjmp breakpoint removal

[Maciej W. Rozycki]

Hi,

 I have observed a crash, where GDB attempts to send a packet to gdbserver 
after the remote target has already exited (and the gdbserver process 
terminated) -- here's the tail of an example remote session transcript:

Sending packet: $vCont;c#a8...Packet received: W00
[Inferior 1 (Remote target) exited normally]
Sending packet: $Hg0#df...

This happens on the MIPS/Linux target while GDB is single-stepping with 
the "step" command.

 I have tracked it down to remote_close calling discard_all_inferiors, 
which in turn eventually calls delete_thread_of_inferior, which calls 
clear_thread_inferior_resources, which calls delete_longjmp_breakpoint, 
which calls remote_remove_breakpoint, which needs to send some packets to 
the remote target to get the breakpoint removed.  But at this point the 
remote file descriptor has already been closed; after a W00 stop reply 
gdbserver does not expect any further input anyway.

 I have looked through ChangeLogs and I believe this is a regression 
caused by:

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

	PR c++/9593:
	* thread.c (clear_thread_inferior_resources): Call
	delete_longjmp_breakpoint.
	[...]

 I have been able to get rid of the crash with the change below, no test 
suite regressions on mips-linux-gnu (remote target).  OK to apply?

2011-12-13  Maciej W. Rozycki  <macro@codesourcery.com>

	gdb/
	* thread.c (clear_thread_inferior_resources): Don't call 
	delete_longjmp_breakpoint if there's no inferior.

  Maciej

gdb-inferior-resources-crash.diff
Index: gdb-fsf-trunk-quilt/gdb/thread.c
===================================================================
--- gdb-fsf-trunk-quilt.orig/gdb/thread.c	2011-11-02 21:28:38.000000000 +0000
+++ gdb-fsf-trunk-quilt/gdb/thread.c	2011-12-13 19:42:25.805620748 +0000
@@ -119,7 +119,8 @@ clear_thread_inferior_resources (struct 
   do_all_intermediate_continuations_thread (tp, 1);
   do_all_continuations_thread (tp, 1);
 
-  delete_longjmp_breakpoint (tp->num);
+  if (!ptid_equal (inferior_ptid, null_ptid))
+    delete_longjmp_breakpoint (tp->num);
 }
 
 static void

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Tom Tromey]

>>>>> "Maciej" == Maciej W Rozycki <macro@codesourcery.com> writes:

Maciej>  I have observed a crash, where GDB attempts to send a packet to
Maciej> gdbserver after the remote target has already exited (and the
Maciej> gdbserver process terminated) -- here's the tail of an example
Maciej> remote session transcript:

Maciej>  I have tracked it down to remote_close calling
Maciej> discard_all_inferiors, which in turn eventually calls
Maciej> delete_thread_of_inferior, which calls
Maciej> clear_thread_inferior_resources, which calls
Maciej> delete_longjmp_breakpoint, which calls remote_remove_breakpoint,
Maciej> which needs to send some packets to the remote target to get the
Maciej> breakpoint removed.  But at this point the remote file
Maciej> descriptor has already been closed; after a W00 stop reply
Maciej> gdbserver does not expect any further input anyway.

You and Keith both came to the same diagnosis, but you have different
patches:

    http://sourceware.org/ml/gdb-patches/2011-11/msg00212.html

... our patch review delays seem to have bitten us this time.
Or anyway bitten the two of you, sorry about that.

I am not sure which approach is better.  I don't think I know enough
about remote.c to say.

Tom

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Maciej W. Rozycki]

On Tue, 20 Dec 2011, Tom Tromey wrote:

> Maciej>  I have observed a crash, where GDB attempts to send a packet to
> Maciej> gdbserver after the remote target has already exited (and the
> Maciej> gdbserver process terminated) -- here's the tail of an example
> Maciej> remote session transcript:
> 
> Maciej>  I have tracked it down to remote_close calling
> Maciej> discard_all_inferiors, which in turn eventually calls
> Maciej> delete_thread_of_inferior, which calls
> Maciej> clear_thread_inferior_resources, which calls
> Maciej> delete_longjmp_breakpoint, which calls remote_remove_breakpoint,
> Maciej> which needs to send some packets to the remote target to get the
> Maciej> breakpoint removed.  But at this point the remote file
> Maciej> descriptor has already been closed; after a W00 stop reply
> Maciej> gdbserver does not expect any further input anyway.
> 
> You and Keith both came to the same diagnosis, but you have different
> patches:
> 
>     http://sourceware.org/ml/gdb-patches/2011-11/msg00212.html
> 
> ... our patch review delays seem to have bitten us this time.
> Or anyway bitten the two of you, sorry about that.
> 
> I am not sure which approach is better.  I don't think I know enough
> about remote.c to say.

 So I have finally got back to it and noticed that this:

http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html

has already fixed the problem -- shall we close gdb/13333 then?  I think 
we should still add Keith's test case to cover any possible future 
breakage in this area -- the fix didn't include any.  I suggest that we 
retrofit the ChangeLog entry with a reference too.

  Maciej

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Pedro Alves]

On 03/07/2012 08:08 PM, Maciej W. Rozycki wrote:

>  So I have finally got back to it and noticed that this:
> 
> http://sourceware.org/ml/gdb-patches/2012-01/msg00701.html
> 
> has already fixed the problem -- shall we close gdb/13333 then?  


If it's fixed, yes.

> I think we should still add Keith's test case to cover any possible future 
> breakage in this area -- the fix didn't include any.  


py-finish-breakpoint.exp was failing against gdbserver without the fix.
I don't mind adding a new test.  Keith's was using gdb_expect, which we
very much like to avoid.  Can it be made to use gdb_test or
gdb_test_multiple?

> I suggest that we retrofit the ChangeLog entry with a reference too.


Please go ahead.

-- 
Pedro Alves

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Keith Seitz]

On 03/07/2012 12:46 PM, Pedro Alves wrote:
> py-finish-breakpoint.exp was failing against gdbserver without the fix.
> I don't mind adding a new test.  Keith's was using gdb_expect, which we
> very much like to avoid.  Can it be made to use gdb_test or
> gdb_test_multiple?

I should think so. The test is pretty simple. I just keep forgetting 
about gdb_test_multiple!

Keith

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Maciej W. Rozycki]

On Wed, 7 Mar 2012, Keith Seitz wrote:

> > py-finish-breakpoint.exp was failing against gdbserver without the fix.

 Hmm, is this test case general enough to be run in all the relevant 
configurations -- i.e. is there no special provision for Python support 
required?

> > I don't mind adding a new test.  Keith's was using gdb_expect, which we
> > very much like to avoid.  Can it be made to use gdb_test or
> > gdb_test_multiple?
> 
> I should think so. The test is pretty simple. I just keep forgetting about
> gdb_test_multiple!

 Keith, would you care updating your test case then?  I'm going to be away 
for two weeks and won't be able to look into it any sooner.

  Maciej

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Keith Seitz]

On 03/09/2012 06:55 PM, Maciej W. Rozycki wrote:
>   Keith, would you care updating your test case then?  I'm going to be away
> for two weeks and won't be able to look into it any sooner.

Sure thing.

Keith

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Keith Seitz]

On 03/09/2012 08:48 PM, Keith Seitz wrote:
> On 03/09/2012 06:55 PM, Maciej W. Rozycki wrote:
>> Keith, would you care updating your test case then? I'm going to be away
>> for two weeks and won't be able to look into it any sooner.
>
> Sure thing.

I've changed the test a little bit to safeguard against looping forever. 
Pedro, does this look acceptable?

Keith

testsuite/ChangeLog
2012-03-09  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.


Index: testsuite/gdb.server/server-run.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.server/server-run.exp,v
retrieving revision 1.12
diff -u -p -r1.12 server-run.exp
--- testsuite/gdb.server/server-run.exp	15 Feb 2012 12:51:17 -0000	1.12
+++ testsuite/gdb.server/server-run.exp	10 Mar 2012 05:08:49 -0000
@@ -51,3 +51,24 @@ 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.
+set max 10
+gdb_test_multiple "next" "step until inferior exits" {
+  -re ".*Inferior 1 \\\(process \[0-9\]+\\\) 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
+    incr max -1
+    if {$max == 0} {
+      unresolved "step until inferior exits"
+    } else {
+      send_gdb "next\n"
+      exp_continue
+    }
+  }
+}

Re: [PATCH] remote: Fix a crash on longjmp breakpoint removal

[Pedro Alves]

On 03/10/2012 05:11 AM, Keith Seitz wrote:

> On 03/09/2012 08:48 PM, Keith Seitz wrote:
>> On 03/09/2012 06:55 PM, Maciej W. Rozycki wrote:
>>> Keith, would you care updating your test case then? I'm going to be away
>>> for two weeks and won't be able to look into it any sooner.
>>
>> Sure thing.
> 
> I've changed the test a little bit to safeguard against looping forever. Pedro, does this look acceptable?
> 


I was going to say there's really nothing target specific in the test, and
that thus it's best to put it under gdb.base/ (well I just did :-) ), but then I
remembered Tom's quite new gdb.base/nextoverexit.exp test that tests the exact
same (albeit in an even simpler form).  I've checked out a tree from just before
my fix, and run it against nextoverexit.exp w/ gdbserver (didn't exist at the
time, so I copied it over), and indeed GDB crashes.

So we don't really need this new test; it's redundant.  Sorry for not realizing
this before you went for the trouble of updating the test...  :-/

-- 
Pedro Alves