[PATCH 2/2] Test GDB connection to GDBserver with no symbol files

[PATCH 2/2] Test GDB connection to GDBserver with no symbol files

[Luis Machado]

This test exercises the scenario where we attempt to connect GDB to GDBserver
in standard remote mode, query the symbol file path, attempt to open said
symbol file on GDB's end and fail, causing the connection to drop abruptly.

I wish i could make all the other PASSes go away and only keep the last test,
as that is the most important one.  As it is, we get full passes for a patched
GDB and 3 PASS/1 FAIL for an unpatched GDB.

Regression-tested on x86-64/Ubuntu.

gdb/testsuite/ChangeLog:

2016-02-24  Luis Machado  <lgustavo@codesourcery.com>

	* gdb.server/attach-with-no-symbol.c: New file.
	* gdb.server/attach-with-no-symbol.exp: New file.
---
 gdb/testsuite/gdb.server/attach-with-no-symbol.c   | 25 ++++++++
 gdb/testsuite/gdb.server/attach-with-no-symbol.exp | 68 ++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 gdb/testsuite/gdb.server/attach-with-no-symbol.c
 create mode 100644 gdb/testsuite/gdb.server/attach-with-no-symbol.exp

diff --git a/gdb/testsuite/gdb.server/attach-with-no-symbol.c b/gdb/testsuite/gdb.server/attach-with-no-symbol.c
new file mode 100644
index 0000000..e5e4acc
--- /dev/null
+++ b/gdb/testsuite/gdb.server/attach-with-no-symbol.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 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/>.  */
+
+#include <stdio.h>
+
+int
+main (int argc, char **argv)
+{
+  printf ("Hello world!\n");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/attach-with-no-symbol.exp b/gdb/testsuite/gdb.server/attach-with-no-symbol.exp
new file mode 100644
index 0000000..a6d0d85
--- /dev/null
+++ b/gdb/testsuite/gdb.server/attach-with-no-symbol.exp
@@ -0,0 +1,68 @@
+# This testcase is part of GDB, the GNU debugger.
+
+# Copyright 2016 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 GDB's ability to properly connect to GDBserver with no pre-opened
+# symbol file.  If GDB is buggy, it will drop the connection when
+# it attempts to open the symbol file indicated by GDBserver and fails.
+#
+# This test is only meaningful for standard remote connections.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if { [skip_gdbserver_tests] } {
+    return 0
+}
+
+if { [prepare_for_testing $testfile.exp $testfile $srcfile debug] } {
+    return -1
+}
+
+# Make sure we're disconnected, in case we're testing with an
+# extended-remote board, therefore already connected.
+gdb_test "disconnect" ".*"
+
+# Discard any symbol files that we have opened.
+gdb_test "file" ".*" "Discard symbol table" \
+    {Discard symbol table from `.*'\? \(y or n\) } "y"
+
+set target_exec [gdbserver_download_current_prog]
+
+# Start GDBserver.
+set res [gdbserver_start "" $target_exec]
+
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+
+# Prevent access to the symbol file.
+if {[file exists $binfile]} {
+    system "chmod 000 $binfile"
+}
+
+# Set sysroot to something non-target and possibly also invalid so that GDB is
+# unable to open the symbol file.
+gdb_test_no_output "set sysroot" "Disable downloading of symbol files"
+
+# Connect to GDBserver.
+gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
+
+# Check if GDB succeeded connecting to GDBserver by attempting to detach from
+# the process.
+gdb_test "detach" \
+	 ".*Detaching from program: , process.*Ending remote debugging.*" \
+	 "Connection to GDBserver succeeded"
-- 
1.9.1

[PATCH 1/2] Debugging without a binary (regression)

[Luis Machado]

When we attempt to debug a process using GDBserver in standard remote mode
without a symbol file on GDB's end, we may run into an issue where GDB cuts
the connection attempt short due to an error. The error is caused by not
being able to open a symbol file, like so:

--

(gdb) set sysroot
(gdb) tar rem :2345
Remote debugging using :2345
/proc/23769/exe: Permission denied.
(gdb) i r
The program has no registers now.
(gdb)

It should've been like this:

(gdb) set sysroot
(gdb) tar rem :2345
Remote debugging using :2345
0xf7ddb2d0 in ?? ()
(gdb) i r
eax            0x0  0
ecx            0x0  0
edx            0x0  0
ebx            0x0  0
esp            0xffffdfa0 0xffffdfa0
ebp            0x0  0x0
esi            0x0  0
edi            0x0  0
eip            0xf7ddb2d0 0xf7ddb2d0
eflags         0x200  [ IF ]
cs             0x33 51
ss             0x2b 43
ds             0x0  0
es             0x0  0
fs             0x0  0
gs             0x0  0
(gdb)

This is caused by a couple of function calls within exec_file_locate_attach
that can potentially throw errors.

The following patch guards both exec_file_attach and symbol_file_add_main to
prevent the errors from disrupting the connection process.

There was also a case where native GDB tripped on this problem, but it was
mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d.

Regression-tested on x86-64/Ubuntu.

gdb/ChangeLog:

2016-02-24  Luis Machado  <lgustavo@codesourcery.com>

	* exec.c (exec_file_locate_attach): Guard a couple functions
	that can throw errors.
---
 gdb/exec.c | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 90811c0..3f77b3d 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty)
 
   old_chain = make_cleanup (xfree, full_exec_path);
 
-  exec_file_attach (full_exec_path, from_tty);
-  symbol_file_add_main (full_exec_path, from_tty);
+  /* exec_file_attach and symbol_file_add_main may throw an error if the file
+     cannot be opened either locally or remotely.  This happens when, for
+     example, GDB connects to a GDBserver that is running on a different
+     filesystem and the sysroot is set to non-target-based (no "target:").
+
+     Then GDB will neither load the binary from the target nor be able to
+     load a binary from the local filesystem (it may not exist in the local
+     filesystem in the same path as in the remote filesystem).
+
+     Even without a binary, the remote-based debugging session should
+     continue normally instead of ending abruptly.  Hence we catch thrown
+     errors/exceptions in the following code.  */
+  TRY
+    {
+      exec_file_attach (full_exec_path, from_tty);
+    }
+  CATCH (err, RETURN_MASK_ALL)
+    {
+    }
+  END_CATCH
+
+  TRY
+    {
+      symbol_file_add_main (full_exec_path, from_tty);
+    }
+  CATCH (err, RETURN_MASK_ALL)
+    {
+    }
+  END_CATCH
 
   do_cleanups (old_chain);
 }
-- 
1.9.1

Re: [PATCH 1/2] Debugging without a binary (regression)

[Pedro Alves]

On 02/24/2016 02:26 PM, Luis Machado wrote:
> When we attempt to debug a process using GDBserver in standard remote mode
> without a symbol file on GDB's end, we may run into an issue where GDB cuts
> the connection attempt short due to an error. The error is caused by not
> being able to open a symbol file, like so:
> 
> --
> 
> (gdb) set sysroot
> (gdb) tar rem :2345
> Remote debugging using :2345
> /proc/23769/exe: Permission denied.
> (gdb) i r
> The program has no registers now.
> (gdb)
> 
> It should've been like this:
> 
> (gdb) set sysroot
> (gdb) tar rem :2345
> Remote debugging using :2345
> 0xf7ddb2d0 in ?? ()
> (gdb) i r
> eax            0x0  0
> ecx            0x0  0
> edx            0x0  0
> ebx            0x0  0
> esp            0xffffdfa0 0xffffdfa0
> ebp            0x0  0x0
> esi            0x0  0
> edi            0x0  0
> eip            0xf7ddb2d0 0xf7ddb2d0
> eflags         0x200  [ IF ]
> cs             0x33 51
> ss             0x2b 43
> ds             0x0  0
> es             0x0  0
> fs             0x0  0
> gs             0x0  0
> (gdb)
> 
> This is caused by a couple of function calls within exec_file_locate_attach
> that can potentially throw errors.
> 
> The following patch guards both exec_file_attach and symbol_file_add_main to
> prevent the errors from disrupting the connection process.
> 
> There was also a case where native GDB tripped on this problem, but it was
> mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d.
> 
> Regression-tested on x86-64/Ubuntu.
> 
> gdb/ChangeLog:
> 
> 2016-02-24  Luis Machado  <lgustavo@codesourcery.com>
> 
> 	* exec.c (exec_file_locate_attach): Guard a couple functions
> 	that can throw errors.
> ---
>  gdb/exec.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/exec.c b/gdb/exec.c
> index 90811c0..3f77b3d 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty)
>  
>    old_chain = make_cleanup (xfree, full_exec_path);
>  
> -  exec_file_attach (full_exec_path, from_tty);
> -  symbol_file_add_main (full_exec_path, from_tty);
> +  /* exec_file_attach and symbol_file_add_main may throw an error if the file
> +     cannot be opened either locally or remotely.  This happens when, for
> +     example, GDB connects to a GDBserver that is running on a different
> +     filesystem and the sysroot is set to non-target-based (no "target:").

The second sentence no longer makes sense after Gary's change.

I think it should read:

  This happens for example, when the file is first found in the local
  sysroot (above), and then disappears (a TOCTOU race), or when it doesn't
  exist in the target filesystem, or when the file does exist, but
  is not readable.

> +
> +     Then GDB will neither load the binary from the target nor be able to
> +     load a binary from the local filesystem (it may not exist in the local
> +     filesystem in the same path as in the remote filesystem).

No longer makes sense either.

> +
> +     Even without a binary, the remote-based debugging session should
> +     continue normally instead of ending abruptly.  Hence we catch thrown
> +     errors/exceptions in the following code.  */
> +  TRY
> +    {
> +      exec_file_attach (full_exec_path, from_tty);
> +    }
> +  CATCH (err, RETURN_MASK_ALL)

This should be RETURN_MASK_ERROR instead.  Silently swallowing ctrl-c is
rarely the right thing to do, if ever.  A ctrl-c during the
initial remote connection is supposed to bring down the connection
immediately:

~~~
  /* Start the remote connection.  If error() or QUIT, discard this
     target (we'd otherwise be in an inconsistent state) and then
     propogate the error on up the exception chain.  This ensures that
     the caller doesn't stumble along blindly assuming that the
     function succeeded.
~~~

static void
remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
{
  struct remote_state *rs = get_remote_state ();
  struct packet_config *noack_config;
  char *wait_status = NULL;

  immediate_quit++;		/* Allow user to interrupt it.  */
  QUIT;
~~~


We should probably similarly completely tear down an incomplete
attach interrupted by ctrl-c too, but that's a separate issue.

> +    {
> +    }
> +  END_CATCH
> +
> +  TRY
> +    {
> +      symbol_file_add_main (full_exec_path, from_tty);
> +    }
> +  CATCH (err, RETURN_MASK_ALL)
> +    {
> +    }

Throwing is bad, but, should we warn, including
the exception message?

> +  END_CATCH
>  
>    do_cleanups (old_chain);
>  }
> 

Thanks,
Pedro Alves

Re: [PATCH 2/2] Test GDB connection to GDBserver with no symbol files

[Pedro Alves]

On 02/24/2016 02:26 PM, Luis Machado wrote:
> This test exercises the scenario where we attempt to connect GDB to GDBserver
> in standard remote mode, query the symbol file path, attempt to open said
> symbol file on GDB's end and fail, causing the connection to drop abruptly.
> 
> I wish i could make all the other PASSes go away and only keep the last test,
> as that is the most important one.  

You can pass an empty string as test message to gdb_test, etc. to suppress it,
but I don't like doing that, because then if one of those setup steps fails,
it fails silently.  I think that if we want to make that work nicely, it
should be based on scoping the setup steps with something similar to
with_test_prefix:

setup_tests_scope {
   gdb_test_no_output "set sysroot" "clear sysroot"
   ...
}

gdb_test ... "real test"

Tests run within setup_tests_scope scope would pass silently, but fail
loudly.

> 
> diff --git a/gdb/testsuite/gdb.server/attach-with-no-symbol.exp b/gdb/testsuite/gdb.server/attach-with-no-symbol.exp
> new file mode 100644
> index 0000000..a6d0d85
> --- /dev/null
> +++ b/gdb/testsuite/gdb.server/attach-with-no-symbol.exp

I think this should be "connect", not "attach".

> @@ -0,0 +1,68 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2016 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 GDB's ability to properly connect to GDBserver with no pre-opened
> +# symbol file.  If GDB is buggy, it will drop the connection when
> +# it attempts to open the symbol file indicated by GDBserver and fails.
> +#
> +# This test is only meaningful for standard remote connections.
> +
> +load_lib gdbserver-support.exp
> +
> +standard_testfile
> +
> +if { [skip_gdbserver_tests] } {
> +    return 0
> +}
> +
> +if { [prepare_for_testing $testfile.exp $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +# Make sure we're disconnected, in case we're testing with an
> +# extended-remote board, therefore already connected.
> +gdb_test "disconnect" ".*"
> +
> +# Discard any symbol files that we have opened.
> +gdb_test "file" ".*" "Discard symbol table" \
> +    {Discard symbol table from `.*'\? \(y or n\) } "y"
> +
> +set target_exec [gdbserver_download_current_prog]
> +
> +# Start GDBserver.
> +set res [gdbserver_start "" $target_exec]
> +
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +
> +# Prevent access to the symbol file.
> +if {[file exists $binfile]} {
> +    system "chmod 000 $binfile"
> +}

Seems like this won't work with remote-host testing?

> +
> +# Set sysroot to something non-target and possibly also invalid so that GDB is
> +# unable to open the symbol file.
> +gdb_test_no_output "set sysroot" "Disable downloading of symbol files"

It's more usual for test messages to start with lower case.

> +
> +# Connect to GDBserver.
> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
> +
> +# Check if GDB succeeded connecting to GDBserver by attempting to detach from
> +# the process.
> +gdb_test "detach" \
> +	 ".*Detaching from program: , process.*Ending remote debugging.*" \
> +	 "Connection to GDBserver succeeded"
> 

Likewise.

How about moving the body of the test to a procedure, and then
also testing deleting the file instead of making it unreadable?

Wouldn't the chmod make gdb also fail to load the file with the default
target: sysroot, since gdbserver would fail to read it?  I think
that should also be a test axis.

Thanks,
Pedro Alves

Re: [PATCH 1/2] Debugging without a binary (regression)

[Luis Machado]

On 02/24/2016 10:06 PM, Pedro Alves wrote:
> On 02/24/2016 02:26 PM, Luis Machado wrote:
>> When we attempt to debug a process using GDBserver in standard remote mode
>> without a symbol file on GDB's end, we may run into an issue where GDB cuts
>> the connection attempt short due to an error. The error is caused by not
>> being able to open a symbol file, like so:
>>
>> --
>>
>> (gdb) set sysroot
>> (gdb) tar rem :2345
>> Remote debugging using :2345
>> /proc/23769/exe: Permission denied.
>> (gdb) i r
>> The program has no registers now.
>> (gdb)
>>
>> It should've been like this:
>>
>> (gdb) set sysroot
>> (gdb) tar rem :2345
>> Remote debugging using :2345
>> 0xf7ddb2d0 in ?? ()
>> (gdb) i r
>> eax            0x0  0
>> ecx            0x0  0
>> edx            0x0  0
>> ebx            0x0  0
>> esp            0xffffdfa0 0xffffdfa0
>> ebp            0x0  0x0
>> esi            0x0  0
>> edi            0x0  0
>> eip            0xf7ddb2d0 0xf7ddb2d0
>> eflags         0x200  [ IF ]
>> cs             0x33 51
>> ss             0x2b 43
>> ds             0x0  0
>> es             0x0  0
>> fs             0x0  0
>> gs             0x0  0
>> (gdb)
>>
>> This is caused by a couple of function calls within exec_file_locate_attach
>> that can potentially throw errors.
>>
>> The following patch guards both exec_file_attach and symbol_file_add_main to
>> prevent the errors from disrupting the connection process.
>>
>> There was also a case where native GDB tripped on this problem, but it was
>> mostly fixed by bf74e428bca61022bd5cdf6bf28789a184748b4d.
>>
>> Regression-tested on x86-64/Ubuntu.
>>
>> gdb/ChangeLog:
>>
>> 2016-02-24  Luis Machado  <lgustavo@codesourcery.com>
>>
>> 	* exec.c (exec_file_locate_attach): Guard a couple functions
>> 	that can throw errors.
>> ---
>>   gdb/exec.c | 31 +++++++++++++++++++++++++++++--
>>   1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/exec.c b/gdb/exec.c
>> index 90811c0..3f77b3d 100644
>> --- a/gdb/exec.c
>> +++ b/gdb/exec.c
>> @@ -176,8 +176,35 @@ exec_file_locate_attach (int pid, int from_tty)
>>
>>     old_chain = make_cleanup (xfree, full_exec_path);
>>
>> -  exec_file_attach (full_exec_path, from_tty);
>> -  symbol_file_add_main (full_exec_path, from_tty);
>> +  /* exec_file_attach and symbol_file_add_main may throw an error if the file
>> +     cannot be opened either locally or remotely.  This happens when, for
>> +     example, GDB connects to a GDBserver that is running on a different
>> +     filesystem and the sysroot is set to non-target-based (no "target:").
>
> The second sentence no longer makes sense after Gary's change.
>
> I think it should read:
>
>    This happens for example, when the file is first found in the local
>    sysroot (above), and then disappears (a TOCTOU race), or when it doesn't
>    exist in the target filesystem, or when the file does exist, but
>    is not readable.
>

Sounds good. Updated in the next version.

>> +
>> +     Then GDB will neither load the binary from the target nor be able to
>> +     load a binary from the local filesystem (it may not exist in the local
>> +     filesystem in the same path as in the remote filesystem).
>
> No longer makes sense either.
>

Updated as well.

>> +
>> +     Even without a binary, the remote-based debugging session should
>> +     continue normally instead of ending abruptly.  Hence we catch thrown
>> +     errors/exceptions in the following code.  */
>> +  TRY
>> +    {
>> +      exec_file_attach (full_exec_path, from_tty);
>> +    }
>> +  CATCH (err, RETURN_MASK_ALL)
>
> This should be RETURN_MASK_ERROR instead.  Silently swallowing ctrl-c is
> rarely the right thing to do, if ever.  A ctrl-c during the
> initial remote connection is supposed to bring down the connection
> immediately:

Fixed

>> +    {
>> +    }
>> +  END_CATCH
>> +
>> +  TRY
>> +    {
>> +      symbol_file_add_main (full_exec_path, from_tty);
>> +    }
>> +  CATCH (err, RETURN_MASK_ALL)
>> +    {
>> +    }
>
> Throwing is bad, but, should we warn, including
> the exception message?

That's what i originally did with a single try/catch block, but i 
noticed with two of them we are prone to having two warnings of the same 
kind, one after the other. So i ended up leaving them empty.

For example:

warning: 
/home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
Permission denied.^M
warning: 
/home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
Permission denied.^M

Shouldn't we go back to a single block? Checking if both exceptions' 
messages match seems a bit too much.

Re: [PATCH 1/2] Debugging without a binary (regression)

[Pedro Alves]

On 02/25/2016 01:36 AM, Luis Machado wrote:
> That's what i originally did with a single try/catch block, but i 
> noticed with two of them we are prone to having two warnings of the same 
> kind, one after the other. So i ended up leaving them empty.
> 
> For example:
> 
> warning: 
> /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
> Permission denied.^M
> warning: 
> /home/test/build-binutils-gdb/gdb/testsuite/outputs/gdb.server/attach-with-no-symbol/attach-with-no-symbol: 
> Permission denied.^M
> 
> Shouldn't we go back to a single block? 

I'd think not.  Pedantically, the file could fail to load as executable
but load as symbol file.  Also, if both fail, and the errors are different,
having both may give a better hint to the user to fix whatever needs fixing.

> Checking if both exceptions' messages match seems a bit too much.

That doesn't sound so bad to me.

static int
exception_print_same (struct gdb_exception e1, struct gdb_exception e2)
{
  const char *msg1 = e1.message;
  const char *msg2 = e2.message;

  if (msg1 == NULL)
    msg1 = "";
  if (msg2 == NULL)
    msg2 = "";

   return (e1.reason == e2.reason
           && e1.error == e2.error
           && strcmp (e1.message, e2.message) == 0);
}

...
  CATCH (err, ...)
    {
      warn err;
      prev_err = err;
    }
...
  CATCH (err, ...)
    {
      if (!exception_print_same (prev_err, err))
        warn err;
    }

Thanks,
Pedro Alves