diff mbox

Fix logic in exec_file_locate_attach

Message ID 1455815129-14795-1-git-send-email-gbenson@redhat.com
State New
Headers show

Commit Message

Gary Benson Feb. 18, 2016, 5:05 p.m. UTC
Hi all,

This commit fixes an error in exec_file_locate_attach where
the main executable could be loaded from outside the sysroot
if a nonempty, non-"target:" sysroot was set but the discovered
executable filename did not exist in that sysroot and did exist
on the main filesystem.

Before:
  gdb -q \
    -ex "set sysroot /whatever" \
    -ex "target remote | gdbserver - /bin/ls"
  ...
  Reading symbols from /bin/ls...(no debugging symbols found)...done.

After:
  gdb -q \
    -ex "set sysroot /whatever" \
    -ex "target remote | gdbserver - /bin/ls"
  ...
  /bin/ls: in sysroot "/whatever": No such file or directory.

Built and regtested on RHEL 6.6 x86_64.

Ok to commit?

Cheers,
Gary

--
gdb/ChangeLog:

	* exec.c (exec_file_locate_attach): Throw error if
	exec_file_find fails to locate the main executable.

gdb/testsuite/ChangeLog:

	* gdb.base/attach-bad-sysroot.exp: New file.
	* gdb.base/attach-bad-sysroot.c: Likewise.
---
 gdb/ChangeLog                                 |    5 +++
 gdb/exec.c                                    |   13 +++++++--
 gdb/testsuite/ChangeLog                       |    5 +++
 gdb/testsuite/gdb.base/attach-bad-sysroot.c   |   25 +++++++++++++++++
 gdb/testsuite/gdb.base/attach-bad-sysroot.exp |   36 +++++++++++++++++++++++++
 5 files changed, 81 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/attach-bad-sysroot.c
 create mode 100644 gdb/testsuite/gdb.base/attach-bad-sysroot.exp

Comments

Pedro Alves Feb. 18, 2016, 5:28 p.m. UTC | #1
On 02/18/2016 05:05 PM, Gary Benson wrote:

> 
> 	* exec.c (exec_file_locate_attach): Throw error if
> 	exec_file_find fails to locate the main executable.

This goes back to:
  https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html

Why is this an error, that even makes us stop the attach process
halfway, if the case when we don't know the file name is completely
silent? :

void
exec_file_locate_attach (int pid, int from_tty)
{
...
  /* Try to determine a filename from the process itself.  */
  exec_file = target_pid_to_exec_file (pid);
  if (exec_file == NULL)
    return;

> +
> +set test_spawn_id [spawn_wait_for_attach $binfile]
> +set testpid [spawn_id_get_pid $test_spawn_id]
> +
> +set outdir [make_gdb_parallel_path outputs $subdir $testfile]
> +set sysroot ${outdir}/does-not-exist
> +
> +gdb_start
> +gdb_test_no_output "set sysroot $sysroot"

Does this "$sysroot" expand to an absolute path?  If so,
the test message depends on where in the filesystem you
happen to run the testsuite.  So this needs an explicit
test message, or maybe simply:

  gdb_test_no_output "set sysroot /dev/null"

> +gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\."
> +
> +kill_wait_spawned_process $test_spawn_id
> 

Thanks,
Pedro Alves
Gary Benson Feb. 19, 2016, 10:24 a.m. UTC | #2
Pedro Alves wrote:
> On 02/18/2016 05:05 PM, Gary Benson wrote:
> > 	* exec.c (exec_file_locate_attach): Throw error if
> > 	exec_file_find fails to locate the main executable.
> 
> This goes back to:
>   https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html
> 
> Why is this an error, that even makes us stop the attach process
> halfway, if the case when we don't know the file name is completely
> silent? :

Hmmm, I was trying to fix a test failure, but looking at it with fresh
eyes this morning it would've been better to fix up that test as the
resulting state is more usable without a throw.  This is with throw:

  (gdb) set sysroot /whatever
  (gdb) attach 31954
  Attaching to process 31954
  ../attach-bad-sysroot: in sysroot "/whatever": No such file or directory.
  (gdb) bt
  #0  0xb54aca20 in ?? ()
  Backtrace stopped: Cannot access memory at address 0x25ae1df8
  (gdb)

This is without throw:

  (gdb) set sysroot /whatever
  (gdb) attach 31954
  Attaching to process 31954
  warning: Could not load vsyscall page because no executable was specified
  try using the "file" command first.
  0x00000039b54aca20 in ?? ()
  (gdb) bt
  #0  0x00000039b54aca20 in ?? ()
  #1  0x00000039b54ac8b0 in ?? ()
  #2  0x0000000000000000 in ?? ()
  (gdb) 

So without the throw you can backtrace.  Also, not throwing will I
think fix things for Luis without adding TRY..CATCH in remote.c.
I'll rework and resubmit.

Cheers,
Gary
Luis Machado Feb. 19, 2016, 10:32 a.m. UTC | #3
On 02/19/2016 08:24 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 02/18/2016 05:05 PM, Gary Benson wrote:
>>> 	* exec.c (exec_file_locate_attach): Throw error if
>>> 	exec_file_find fails to locate the main executable.
>>
>> This goes back to:
>>    https://sourceware.org/ml/gdb-patches/2016-02/msg00413.html
>>
>> Why is this an error, that even makes us stop the attach process
>> halfway, if the case when we don't know the file name is completely
>> silent? :
>
> Hmmm, I was trying to fix a test failure, but looking at it with fresh
> eyes this morning it would've been better to fix up that test as the
> resulting state is more usable without a throw.  This is with throw:
>
>    (gdb) set sysroot /whatever
>    (gdb) attach 31954
>    Attaching to process 31954
>    ../attach-bad-sysroot: in sysroot "/whatever": No such file or directory.
>    (gdb) bt
>    #0  0xb54aca20 in ?? ()
>    Backtrace stopped: Cannot access memory at address 0x25ae1df8
>    (gdb)
>
> This is without throw:
>
>    (gdb) set sysroot /whatever
>    (gdb) attach 31954
>    Attaching to process 31954
>    warning: Could not load vsyscall page because no executable was specified
>    try using the "file" command first.
>    0x00000039b54aca20 in ?? ()
>    (gdb) bt
>    #0  0x00000039b54aca20 in ?? ()
>    #1  0x00000039b54ac8b0 in ?? ()
>    #2  0x0000000000000000 in ?? ()
>    (gdb)
>
> So without the throw you can backtrace.  Also, not throwing will I
> think fix things for Luis without adding TRY..CATCH in remote.c.
> I'll rework and resubmit.

For the record, i'm re-working my previous patch to handle the case of 
native attaches too, when the file access problem cuts the attachment 
process halfway through (regardless of sysroot use policies).

I'm playing around with the TRY/CATCH block inside 
exec_file_locate_attach as opposed to taking care of its callers (in 
remote.c and infcmd.c).

Currently i'm checking for regressions and writing a testcase. I just 
want to make sure our changes don't overlap in terms of logic.
diff mbox

Patch

diff --git a/gdb/exec.c b/gdb/exec.c
index 0b8c077..8f19871 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -156,9 +156,16 @@  exec_file_locate_attach (int pid, int from_tty)
   /* If gdb_sysroot is not empty and the discovered filename
      is absolute then prefix the filename with gdb_sysroot.  */
   if (*gdb_sysroot != '\0' && IS_ABSOLUTE_PATH (exec_file))
-    full_exec_path = exec_file_find (exec_file, NULL);
-
-  if (full_exec_path == NULL)
+    {
+      full_exec_path = exec_file_find (exec_file, NULL);
+      if (full_exec_path == NULL)
+	{
+	  error (_("%s: in sysroot \"%s\":"
+		   " No such file or directory."),
+		 exec_file, gdb_sysroot);
+	}
+    }
+  else
     {
       /* It's possible we don't have a full path, but rather just a
 	 filename.  Some targets, such as HP-UX, don't provide the
diff --git a/gdb/testsuite/gdb.base/attach-bad-sysroot.c b/gdb/testsuite/gdb.base/attach-bad-sysroot.c
new file mode 100644
index 0000000..e1c1e70
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-bad-sysroot.c
@@ -0,0 +1,25 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2011-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 <unistd.h>
+
+int
+main (void)
+{
+  sleep (600);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/attach-bad-sysroot.exp b/gdb/testsuite/gdb.base/attach-bad-sysroot.exp
new file mode 100644
index 0000000..014da65
--- /dev/null
+++ b/gdb/testsuite/gdb.base/attach-bad-sysroot.exp
@@ -0,0 +1,36 @@ 
+# Copyright (C) 2011-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/>.
+
+if {![can_spawn_for_attach]} {
+    return 0
+}
+
+standard_testfile .c
+if { [build_executable ${testfile}.exp $binfile "${testfile}.c"] == -1 } {
+    untested ${testfile}.exp
+    return -1
+}
+
+set test_spawn_id [spawn_wait_for_attach $binfile]
+set testpid [spawn_id_get_pid $test_spawn_id]
+
+set outdir [make_gdb_parallel_path outputs $subdir $testfile]
+set sysroot ${outdir}/does-not-exist
+
+gdb_start
+gdb_test_no_output "set sysroot $sysroot"
+gdb_test "attach $testpid" "Attaching to process $testpid\r\n.*: No such file or directory\\."
+
+kill_wait_spawned_process $test_spawn_id