[PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[Pedro Alves]

 $ ./gdbserver :1234 blah
 Process blah created; pid = 16471
 Cannot exec blah: No such file or directory.

 Child exited with status 127
 Killing process(es): 16471
 ../../../../src/binutils-gdb/gdb/gdbserver/linux-low.c:920: A problem internal to GDBserver has been detected.
 kill_wait_lwp: Assertion `res > 0' failed.

GDBserver shouldn't even be trying to kill that process.  GDBserver
kills or detaches from all processes on exit, and due to a missing
mourn_inferior call, GDBserver tries to kill the process that it had
already seen exit.

Tested on x86_64 Fedora 20.  New test included.  I emulated what
Windows outputs by hacking an error call in linux_create_inferior.

gdb/gdbserver/ChangeLog:
2015-04-07  Pedro Alves  <palves@redhat.com>

	PR server/18081
	* server.c (start_inferior): If the process exits, mourn it.

gdb/testsuite/ChangeLog:
2015-04-07  Pedro Alves  <palves@redhat.com>

	PR server/18081
	* gdb.server/non-existing-program.exp: New file.
---
 gdb/gdbserver/server.c                            |  2 +
 gdb/testsuite/gdb.server/non-existing-program.exp | 63 +++++++++++++++++++++++
 2 files changed, 65 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/non-existing-program.exp

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 3408ef7..d57674d 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -284,6 +284,8 @@ start_inferior (char **argv)
       current_thread->last_resume_kind = resume_stop;
       current_thread->last_status = last_status;
     }
+  else
+    mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
 
   return signal_pid;
 }
diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
new file mode 100644
index 0000000..f842c44
--- /dev/null
+++ b/gdb/testsuite/gdb.server/non-existing-program.exp
@@ -0,0 +1,63 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test starting gdbserver passing it the name of a non-existing
+# program.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+set gdbserver [find_gdbserver]
+if { $gdbserver == "" } {
+    fail "could not find gdbserver"
+    return
+}
+
+# Fire off gdbserver.  The port doesn't really matter, gdbserver tries
+# to spawn the program before opening the connection.
+set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
+
+set msg "gdbserver exits cleanly"
+set saw_exiting 0
+expect {
+    # This is what we get on ptrace-based targets.
+    -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" {
+	set saw_exiting 1
+	exp_continue
+    }
+    # This is what we get on Windows.
+    -re "Error creating process\r\n\r\nExiting\r\n$" {
+	set saw_exiting 1
+	exp_continue
+    }
+    -re "A problem internal to GDBserver has been detected" {
+	fail "$msg (GDBserver internal error)"
+	wait
+    }
+    eof {
+	gdb_assert $saw_exiting $msg
+	wait
+    }
+    timeout {
+	fail "$msg (timeout)"
+    }
+}
-- 
1.9.3

Re: [PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[Simon Marchi]

On 15-04-07 02:29 PM, Pedro Alves wrote:
>  $ ./gdbserver :1234 blah
>  Process blah created; pid = 16471
>  Cannot exec blah: No such file or directory.
> 
>  Child exited with status 127
>  Killing process(es): 16471
>  ../../../../src/binutils-gdb/gdb/gdbserver/linux-low.c:920: A problem internal to GDBserver has been detected.
>  kill_wait_lwp: Assertion `res > 0' failed.
> 
> GDBserver shouldn't even be trying to kill that process.  GDBserver
> kills or detaches from all processes on exit, and due to a missing
> mourn_inferior call, GDBserver tries to kill the process that it had
> already seen exit.
> 
> Tested on x86_64 Fedora 20.  New test included.  I emulated what
> Windows outputs by hacking an error call in linux_create_inferior.
> 
> gdb/gdbserver/ChangeLog:
> 2015-04-07  Pedro Alves  <palves@redhat.com>
> 
> 	PR server/18081
> 	* server.c (start_inferior): If the process exits, mourn it.
> 
> gdb/testsuite/ChangeLog:
> 2015-04-07  Pedro Alves  <palves@redhat.com>
> 
> 	PR server/18081
> 	* gdb.server/non-existing-program.exp: New file.
> ---
>  gdb/gdbserver/server.c                            |  2 +
>  gdb/testsuite/gdb.server/non-existing-program.exp | 63 +++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.server/non-existing-program.exp
> 
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index 3408ef7..d57674d 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -284,6 +284,8 @@ start_inferior (char **argv)
>        current_thread->last_resume_kind = resume_stop;
>        current_thread->last_status = last_status;
>      }
> +  else
> +    mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
>  
>    return signal_pid;
>  }
> diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
> new file mode 100644
> index 0000000..f842c44
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/non-existing-program.exp
> @@ -0,0 +1,63 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2015 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test starting gdbserver passing it the name of a non-existing
> +# program.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +if { [skip_gdbserver_tests] } {
> +    return 0
> +}
> +
> +set gdbserver [find_gdbserver]
> +if { $gdbserver == "" } {
> +    fail "could not find gdbserver"
> +    return
> +}
> +
> +# Fire off gdbserver.  The port doesn't really matter, gdbserver tries
> +# to spawn the program before opening the connection.
> +set spawn_id [remote_spawn target "$gdbserver stdio non-existing-program"]
> +
> +set msg "gdbserver exits cleanly"
> +set saw_exiting 0
> +expect {
> +    # This is what we get on ptrace-based targets.
> +    -re "stdin/stdout redirected.*No program to debug\r\nExiting\r\n$" {
> +	set saw_exiting 1
> +	exp_continue
> +    }
> +    # This is what we get on Windows.
> +    -re "Error creating process\r\n\r\nExiting\r\n$" {
> +	set saw_exiting 1
> +	exp_continue
> +    }
> +    -re "A problem internal to GDBserver has been detected" {
> +	fail "$msg (GDBserver internal error)"
> +	wait
> +    }
> +    eof {
> +	gdb_assert $saw_exiting $msg
> +	wait
> +    }
> +    timeout {
> +	fail "$msg (timeout)"
> +    }
> +}

It's much better now, thanks!

Re: [PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[Pedro Alves]

On 04/08/2015 08:30 PM, Simon Marchi wrote:

> It's much better now, thanks!

FYI, I've pushed this in now.

Thanks,
Pedro Alves

Re: [PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[Luis Machado]

On 05/06/2015 03:14 PM, Pedro Alves wrote:
> On 04/08/2015 08:30 PM, Simon Marchi wrote:
>
>> It's much better now, thanks!
>
> FYI, I've pushed this in now.

For some reason the next test after that now fails and stops the 
testsuite run.


Running 
../../../gdb-head-ro/gdb/testsuite/gdb.server/non-existing-program.exp ...
Running 
../../../gdb-head-ro/gdb/testsuite/gdb.server/server-exec-info.exp ...
can not find channel named "exp8"
     while executing
"match_max [match_max -d]"
     (procedure "default_gdb_init" line 26)
     invoked from within
"default_gdb_init $test_file_name"
     (procedure "gdb_init" line 83)
     invoked from within
"${tool}_init $test_file_name"
     (procedure "runtest" line 19)
     invoked from within
"runtest $test_name"
     ("foreach" body line 24)
     invoked from within
"foreach test_name $testlist {
		if { ${ignoretests} != "" } {
		    if { 0 <= [lsearch ${ignoretests} [file tail ${test_name}]]} {
			continue
		    }..."
     ("foreach" body line 92)
     invoked from within
"foreach pass $multipass {

	# multipass_name is set for `record_test' to use (see framework.exp).
	if { [lindex $pass 0] != "" } {
	    set multipass_..."
     ("foreach" body line 51)
     invoked from within
"foreach current_target $target_list {
     verbose "target is $current_target"
     set current_target_name $current_target
     set tlist [split $curren..."
     (file "/usr/share/dejagnu/runtest.exp" line 1624)

Re: [PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary

[Pedro Alves]

On 05/08/2015 01:53 PM, Luis Machado wrote:
> On 05/06/2015 03:14 PM, Pedro Alves wrote:
>> On 04/08/2015 08:30 PM, Simon Marchi wrote:
>>
>>> It's much better now, thanks!
>>
>> FYI, I've pushed this in now.
> 
> For some reason the next test after that now fails and stops the 
> testsuite run.

It's probably the setting of spawn_id.  If so, it's the sort of
thing that is only visible on a serial run, and I probably only
tested a parallel run.  I'll take a look.

Thanks,
Pedro Alves

[pushed] Fix sequential gdb test runs (Re: [PATCH] PR server/18081: gdbserver crashes when providing an unexisting binary)

[Pedro Alves]

On 05/08/2015 04:08 PM, Pedro Alves wrote:
> On 05/08/2015 01:53 PM, Luis Machado wrote:
>> On 05/06/2015 03:14 PM, Pedro Alves wrote:
>>> On 04/08/2015 08:30 PM, Simon Marchi wrote:
>>>
>>>> It's much better now, thanks!
>>>
>>> FYI, I've pushed this in now.
>>
>> For some reason the next test after that now fails and stops the 
>> testsuite run.
> 
> It's probably the setting of spawn_id.  If so, it's the sort of
> thing that is only visible on a serial run, and I probably only
> tested a parallel run.  I'll take a look.

Fixed with the patch below.

Though I have to say that the ...

    # Unlike most tests, we have a small number of tests that generate
    # a very large amount of output.  We therefore increase the expect
    # buffer size to be able to contain the entire test output.  This
    # is especially needed by gdb.base/info-macros.exp.
    match_max -d 65536
    # Also set this value for the currently running GDB.
    match_max [match_max -d]

... second match_max here (this is default_gdb_init) looks quite odd to
me.  There should be no GDB running here, as this routine
is called first thing before the test's code is called.  And before we move
on to another test, dejagnu calls gdb_finish, which kills gdb, so not
even sequential runs should end up with spawn_id set to a GDB process here.

If spawn_id is not set, match_max defaults to the $user_spawn_id / stdin/out
channel.  But even if there's a running gdb at this point somehow, the test
that we're about to start certainly kills the running gdb and starts a new
one, so the default set by match_max -d applies to the new gdb and therefore
the second match_max shouldn't matter for anything.

But that was added by:

 https://sourceware.org/ml/gdb-patches/2009-10/msg00054.html

for:

  gdb.server/ext-run.exp: get process list

At the time of that commit, that test in ext-run.exp used gdb_test,
while nowadays it uses gdb_test_sequence (so it's protected from
"internal buffer full" in another way anyway).  Still, I hacked
default_gdb_init to go as low as:

    match_max 10

and reverted back to using gdb_test in that test in ext-run.exp, but,
ext-run.exp didn't break.  So I'm puzzled why that was ever needed.

---
From a4674e4efc0c93abd2865d5cf97da710fa3affae Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 8 May 2015 18:06:46 +0100
Subject: [PATCH] Fix sequential gdb test runs

Sequential test runs are stopping prematurely like this:

 $ make check RUNTESTFLAGS="non-existing-program.exp server-exec-info.exp"

 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.server/non-existing-program.exp ...
 Running /home/pedro/gdb/mygit/src/gdb/testsuite/gdb.server/server-exec-info.exp ...
 can not find channel named "exp6"
     while executing
 "match_max [match_max -d]"
     (procedure "default_gdb_init" line 26)
     invoked from within
 "default_gdb_init $test_file_name"
     (procedure "gdb_init" line 83)
     invoked from within
 "${tool}_init $test_file_name"
     (procedure "runtest" line 18)
     invoked from within
 "runtest $test_name"
     ("foreach" body line 42)
     invoked from within
 ...
 make[2]: *** [check-single] Error 1
 make[2]: Leaving directory `/home/pedro/gdb/mygit/build/gdb/testsuite'
 make[1]: *** [check] Error 2
 make[1]: Leaving directory `/home/pedro/gdb/mygit/build/gdb/testsuite'
 make: *** [check] Error 2

default_gdb_init has this:

    # Unlike most tests, we have a small number of tests that generate
    # a very large amount of output.  We therefore increase the expect
    # buffer size to be able to contain the entire test output.  This
    # is especially needed by gdb.base/info-macros.exp.
    match_max -d 65536
    # Also set this value for the currently running GDB.
    match_max [match_max -d]

It's the second match_max that is erroring.  As that call does not
specify an explicit channel name with -i, expect defaults to
$spawn_id, which is pointing at a channel that is already gone.  (If
the spawn_id variable is not set, match_max defaults to
$user_spawn_id / stdin/out).

gdb/testsuite/ChangeLog:
2015-05-08  Pedro Alves  <palves@redhat.com>

	* gdb.server/non-existing-program.exp: Unset spawn_id.
---
 gdb/testsuite/ChangeLog                           | 4 ++++
 gdb/testsuite/gdb.server/non-existing-program.exp | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 93a0e7f..4064b28 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2015-05-08  Pedro Alves  <palves@redhat.com>
+
+	* gdb.server/non-existing-program.exp: Unset spawn_id.
+
 2015-05-08  Siva Chandra Reddy  <sivachandra@google.com>
 
 	PR python/18291
diff --git a/gdb/testsuite/gdb.server/non-existing-program.exp b/gdb/testsuite/gdb.server/non-existing-program.exp
index f842c44..63a9cb7 100644
--- a/gdb/testsuite/gdb.server/non-existing-program.exp
+++ b/gdb/testsuite/gdb.server/non-existing-program.exp
@@ -61,3 +61,7 @@ expect {
 	fail "$msg (timeout)"
     }
 }
+
+# expect defaults to spawn_id in many places.  Avoid confusing any
+# following code.
+unset spawn_id
-- 
1.9.3