[PATCH][gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

[PATCH][gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

[Tom de Vries]

Hi,

with the test-case contained in this patch and compiled for debug we run into
a segfault with trunk gdb:
...
$ gdb catch-follow-exec -batch -ex "catch exec" \
  -ex "set follow-exec-mode new" -ex "run" -ex "info prog"
Catchpoint 1 (exec)
process xxx is executing new program: /usr/bin/ls
[New inferior 2 (process 0)]
[New process xxx]

Thread 2.1 "ls" hit Catchpoint 1 (exec'd /usr/bin/ls), in _start () from
  /lib64/ld-linux-x86-64.so.2
Segmentation fault (core dumped)
...

The patch fixes the segfault by returning an error in info_program_command
if get_last_target_status returns minus_one_ptid.

The test-case is non-standard, because the standard approach runs into
PR23020, a problem with gdb going to the background.

Build and reg-tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

2018-07-20  Tom de Vries  <tdevries@suse.de>

	PR breakpoints/23366
	* infcmd.c (info_program_command): Handle ptid == minus_one_ptid.

	* gdb.base/catch-follow-exec.c: New test.
	* gdb.base/catch-follow-exec.exp: New file.

---
 gdb/infcmd.c                                 |  2 +-
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 10 +++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 56 ++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 821bcc6544..74d5956765 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2091,7 +2091,7 @@ info_program_command (const char *args, int from_tty)
       get_last_target_status (&ptid, &ws);
     }
 
-  if (ptid == null_ptid)
+  if (ptid == null_ptid || ptid == minus_one_ptid)
     error (_("No selected thread."));
 
   thread_info *tp = find_thread_ptid (ptid);
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
new file mode 100644
index 0000000000..fa68a2a34e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  char *exec_args[] = { "/bin/ls", "ppp", NULL };
+  execve (exec_args[0], exec_args, NULL);
+}
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
new file mode 100644
index 0000000000..03611e9517
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -0,0 +1,56 @@
+# Copyright 2018 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/>.
+
+# Check whether finish respects the print pretty user setting when printing the
+# function result.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return -1
+}
+
+if { ![file exists /bin/bash] } {
+    unsupported "no bash"
+}
+
+if { ![file exists /bin/ls] } {
+    unsupported "no ls"
+}
+
+proc catch_follow_exec { } {
+    global binfile
+    global GDB
+
+    set test "catch-follow-exec"
+
+    append FLAGS " \"$binfile\""
+    append FLAGS " -batch"
+    append FLAGS " -ex \"catch exec\""
+    append FLAGS " -ex \"set follow-exec-mode new\""
+    append FLAGS " -ex \"run\""
+    append FLAGS " -ex \"info prog\""
+
+    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
+    send_log "$catchlog\n"
+
+    if { [regexp {No selected thread} $catchlog] } {
+	pass $test
+    } else {
+	fail $test
+    }
+}
+
+catch_follow_exec

Re: [PATCH][gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> The patch fixes the segfault by returning an error in info_program_command
Tom> if get_last_target_status returns minus_one_ptid.

Tom> The test-case is non-standard, because the standard approach runs into
Tom> PR23020, a problem with gdb going to the background.

Is it possible to fix this part first?
Otherwise it seems to me that the test case has some issues.

Also, is the bug dependent on -batch?  This wasn't clear to me, but I
guess if it is then the approach to the test case makes more sense.

Tom> 	PR breakpoints/23366
Tom> 	* infcmd.c (info_program_command): Handle ptid == minus_one_ptid.

This part looks ok to me.

Tom> +if { ![file exists /bin/bash] } {
...
Tom> +if { ![file exists /bin/ls] } {
...
Tom> +    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog

I think this sort of thing can't be done when doing remote host testing,
so some sort of check is needed for that.  I don't remember any more
exactly how this is done but there should be other examples in the tree.

Tom> +if { ![file exists /bin/bash] } {
Tom> +    unsupported "no bash"
Tom> +}

Also there has to be a "return" after the "unsupported"; unsupported
just logs a message and without the return, the rest of the test will
still be run.

Tom

Re: [PATCH][gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1742 bytes --]

On 07/25/2018 09:17 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> The patch fixes the segfault by returning an error in info_program_command
> Tom> if get_last_target_status returns minus_one_ptid.
> 
> Tom> The test-case is non-standard, because the standard approach runs into
> Tom> PR23020, a problem with gdb going to the background.
> 
> Is it possible to fix this part first?

I've investigated a bit, but I think I need serious time to be able to
fix this, I'm not familiar with the tty interface.

> Otherwise it seems to me that the test case has some issues.
> 
> Also, is the bug dependent on -batch?

No, that's the way to run the test-case without running into PR23368
(gdb going to the background).

>  This wasn't clear to me, but I
> guess if it is then the approach to the test case makes more sense.
> 
> Tom> 	PR breakpoints/23366
> Tom> 	* infcmd.c (info_program_command): Handle ptid == minus_one_ptid.
> 
> This part looks ok to me.
> 
> Tom> +if { ![file exists /bin/bash] } {
> ...
> Tom> +if { ![file exists /bin/ls] } {
> ...
> Tom> +    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
> 

I'm now using "remote_file target exists".

> I think this sort of thing can't be done when doing remote host testing,
> so some sort of check is needed for that.  I don't remember any more
> exactly how this is done but there should be other examples in the tree.
> 
> Tom> +if { ![file exists /bin/bash] } {
> Tom> +    unsupported "no bash"
> Tom> +}
> 
> Also there has to be a "return" after the "unsupported"; unsupported
> just logs a message and without the return, the rest of the test will
> still be run.
> 

Updated accordingly and committed as attached.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-breakpoints-Fix-sigsegv-in-info-prog-at-exec-catchpoint.patch --]
[-- Type: text/x-patch, Size: 4006 bytes --]

[gdb/breakpoints] Fix sigsegv in info prog at exec catchpoint

With the test-case contained in this patch and compiled for debug we run into
a segfault with trunk gdb:
...
$ gdb catch-follow-exec -batch -ex "catch exec" \
  -ex "set follow-exec-mode new" -ex "run" -ex "info prog"
Catchpoint 1 (exec)
process xxx is executing new program: /usr/bin/ls
[New inferior 2 (process 0)]
[New process xxx]

Thread 2.1 "ls" hit Catchpoint 1 (exec'd /usr/bin/ls), in _start () from
  /lib64/ld-linux-x86-64.so.2
Segmentation fault (core dumped)
...

The patch fixes the segfault by returning an error in info_program_command
if get_last_target_status returns minus_one_ptid.

The test-case is non-standard, because the standard approach runs into
PR23368, a problem with gdb going to the background.

Build and reg-tested on x86_64-linux.

2018-07-20  Tom de Vries  <tdevries@suse.de>

	PR breakpoints/23366
	* infcmd.c (info_program_command): Handle ptid == minus_one_ptid.

	* gdb.base/catch-follow-exec.c: New test.
	* gdb.base/catch-follow-exec.exp: New file.

---
 gdb/infcmd.c                                 |  2 +-
 gdb/testsuite/gdb.base/catch-follow-exec.c   | 10 +++++
 gdb/testsuite/gdb.base/catch-follow-exec.exp | 58 ++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 821bcc6544..74d5956765 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2091,7 +2091,7 @@ info_program_command (const char *args, int from_tty)
       get_last_target_status (&ptid, &ws);
     }
 
-  if (ptid == null_ptid)
+  if (ptid == null_ptid || ptid == minus_one_ptid)
     error (_("No selected thread."));
 
   thread_info *tp = find_thread_ptid (ptid);
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.c b/gdb/testsuite/gdb.base/catch-follow-exec.c
new file mode 100644
index 0000000000..fa68a2a34e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.c
@@ -0,0 +1,10 @@
+#include <stdio.h>
+#include <string.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  char *exec_args[] = { "/bin/ls", "ppp", NULL };
+  execve (exec_args[0], exec_args, NULL);
+}
diff --git a/gdb/testsuite/gdb.base/catch-follow-exec.exp b/gdb/testsuite/gdb.base/catch-follow-exec.exp
new file mode 100644
index 0000000000..0e32ed4a6f
--- /dev/null
+++ b/gdb/testsuite/gdb.base/catch-follow-exec.exp
@@ -0,0 +1,58 @@
+# Copyright 2018 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/>.
+
+# Check whether finish respects the print pretty user setting when printing the
+# function result.
+
+standard_testfile
+
+if {[build_executable "failed to prepare" $testfile $srcfile debug] == -1} {
+    return -1
+}
+
+if { ![remote_file target exists /bin/bash] } {
+    unsupported "no bash"
+    return
+}
+
+if { ![remote_file target exists /bin/ls] } {
+    unsupported "no ls"
+    return
+}
+
+proc catch_follow_exec { } {
+    global binfile
+    global GDB
+
+    set test "catch-follow-exec"
+
+    append FLAGS " \"$binfile\""
+    append FLAGS " -batch"
+    append FLAGS " -ex \"catch exec\""
+    append FLAGS " -ex \"set follow-exec-mode new\""
+    append FLAGS " -ex \"run\""
+    append FLAGS " -ex \"info prog\""
+
+    catch {exec /bin/bash -c "$GDB $FLAGS"} catchlog
+    send_log "$catchlog\n"
+
+    if { [regexp {No selected thread} $catchlog] } {
+	pass $test
+    } else {
+	fail $test
+    }
+}
+
+catch_follow_exec