[review] Make sure we have a valid target on top when pushing a new target

[review] Make sure we have a valid target on top when pushing a new target

[Sergio Durigan Junior (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................

Make sure we have a valid target on top when pushing a new target

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117

A segfault can happen in a specific scenario when using TUI + a
corefile, as explained in the bug mentioned above.  The problem
happens when opening a corefile on GDB:

  $ gdb ./core program

entering TUI (C-x a), and then issuing a "run" command.  GDB segfaults
with the following stack trace:

  (top-gdb) bt
  #0  0x00000000004cd5da in target_ops::shortname (this=0x0) at ../../binutils-gdb/gdb/target.h:449
  #1  0x0000000000ac08fb in target_shortname () at ../../binutils-gdb/gdb/target.h:1323
  #2  0x0000000000ac09ae in tui_locator_window::make_status_line[abi:cxx11]() const (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:86
  #3  0x0000000000ac1043 in tui_locator_window::rerender (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:231
  #4  0x0000000000ac1632 in tui_show_locator_content () at ../../binutils-gdb/gdb/tui/tui-stack.c:369
  #5  0x0000000000ac63b6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at ../../binutils-gdb/gdb/tui/tui.c:321
  #6  0x0000000000aaf9be in tui_inferior_exit (inf=0x2d446a0) at ../../binutils-gdb/gdb/tui/tui-hooks.c:181
  #7  0x000000000044cddf in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&) (__functor=..., __args#0=@0x7fffffffd650: 0x2d446a0)
      at /usr/include/c++/9/bits/std_function.h:300
  #8  0x0000000000757db9 in std::function<void (inferior*)>::operator()(inferior*) const (this=0x2cf3168, __args#0=0x2d446a0) at /usr/include/c++/9/bits/std_function.h:690
  #9  0x0000000000757876 in gdb::observers::observable<inferior*>::notify (this=0x23de0c0 <gdb::observers::inferior_exit>, args#0=0x2d446a0)
      at ../../binutils-gdb/gdb/gdbsupport/observable.h:106
  #10 0x000000000075532d in exit_inferior_1 (inftoex=0x2d446a0, silent=1) at ../../binutils-gdb/gdb/inferior.c:191
  #11 0x0000000000755460 in exit_inferior_silent (inf=0x2d446a0) at ../../binutils-gdb/gdb/inferior.c:234
  #12 0x000000000059f47c in core_target::close (this=0x2d68590) at ../../binutils-gdb/gdb/corelow.c:265
  #13 0x0000000000a7688c in target_close (targ=0x2d68590) at ../../binutils-gdb/gdb/target.c:3293
  #14 0x0000000000a63d74 in target_stack::push (this=0x23e1800 <g_target_stack>, t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:568
  #15 0x0000000000a63dbf in push_target (t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:583
  #16 0x0000000000748088 in inf_ptrace_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/inf-ptrace.c:128
  #17 0x0000000000795ccb in linux_nat_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/linux-nat.c:1094
  #18 0x000000000074eae9 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../binutils-gdb/gdb/infcmd.c:639
  ...

The problem happens because 'tui_locator_window::make_status_line'
needs the value of 'target_shortname' in order to update the status
line.  'target_shortname' is a macro which expands to:

  #define	target_shortname	(current_top_target ()->shortname ())

and, in our scenario, 'current_top_target ()' returns NULL, which
obviously causes a segfault.  But why does it return NULL, since,
according to its comment on target.h, it should never do that?

What is happening is that we're being caught in the middle of a
"target switch".  We had the 'core_target' on top, because we were
inspecting a corefile, but when the user decided to invoke "run" GDB
had to actually create the inferior, which ends up detecting that we
have a target already, and tries to close it (from target.c):

  /* See target.h.  */

  void
  target_stack::push (target_ops *t)
  {
    /* If there's already a target at this stratum, remove it.  */
    strata stratum = t->stratum ();

    if (m_stack[stratum] != NULL)
      {
	target_ops *prev = m_stack[stratum];
	m_stack[stratum] = NULL;
	target_close (prev); // <-- here
      }
  ...

When the current target ('core_target') is being closed, it checks for
possible observers registered with it and calls them.  TUI is one of
those observers, it gets called, tries to update the status line, and
GDB crashes.

The real problem is that we are calling 'target_close' too soon,
before we actually have the new target on top of the stack.
Interestingly, this scenario is covered in 'target_stack::unpush', but
for some reason we are not calling it here.

It is unclear whether we should be calling '::unpush' here or not, but
in any case I decided to just move the 'target_close' call further
down, after we already have a valid target occupying its place.

This patch has been tested on the Buildbot and no regressions have
been found.  I'm also submitting a testcase for it.

gdb/ChangeLog:
2019-11-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* target.c (target_stack::push): Call 'target_close' only
	after adding the new target to the stratum.

gdb/testsuite/ChangeLog:
2019-11-01  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* gdb.tui/segfault-corefile-run.exp: New file.

Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
---
M gdb/target.c
A gdb/testsuite/gdb.tui/segfault-corefile-run.exp
2 files changed, 67 insertions(+), 5 deletions(-)



diff --git a/gdb/target.c b/gdb/target.c
index 7c9befc..bcd7a64 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -560,17 +560,18 @@
 {
   /* If there's already a target at this stratum, remove it.  */
   strata stratum = t->stratum ();
+  target_ops *prev = nullptr;
 
   if (m_stack[stratum] != NULL)
-    {
-      target_ops *prev = m_stack[stratum];
-      m_stack[stratum] = NULL;
-      target_close (prev);
-    }
+    prev = m_stack[stratum];
 
   /* Now add the new one.  */
   m_stack[stratum] = t;
 
+  /* And close the previous one, if it exists.  */
+  if (prev != nullptr)
+    target_close (prev);
+
   if (m_top < stratum)
     m_top = stratum;
 }
diff --git a/gdb/testsuite/gdb.tui/segfault-corefile-run.exp b/gdb/testsuite/gdb.tui/segfault-corefile-run.exp
new file mode 100644
index 0000000..05fcc24
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/segfault-corefile-run.exp
@@ -0,0 +1,61 @@
+# Copyright 2019 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 whether we can load a corefile, enable TUI and then invoke
+# "run" without having a segfault.
+#
+# Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+set core "${testfile}.core"
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Only run on native boards.
+if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+    untested "not supported"
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+if { ![gdb_gcore_cmd "$core" "save a corefile"] } {
+    untested "could not generate a corefile"
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+}
+
+set text [Term::get_all_lines]
+gdb_assert {![string match "No Source Available" $text]} \
+    "initial source listing"
+
+Term::command "core-file $core"
+Term::check_contents "load corefile" "21 *return 0"
+
+Term::command "run"
+Term::check_contents "run until the end" \
+    "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: newchange

[review] Make sure we have a valid target on top when pushing a new target

[Sergio Durigan Junior (Code Review)]

Sergio Durigan Junior has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 1:

Pedro, could you take a look at this, please?


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Mon, 04 Nov 2019 17:02:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review] Make sure we have a valid target on top when pushing a new target

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

Thanks for the fix.  I don't think it's the right one, but the right one is in the area.  The test looks good, but I think you should rename the file.  More details in the comments.

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1//COMMIT_MSG 
Commit Message:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1//COMMIT_MSG@7 
PS1, Line 7: 
 2 | Author:     Sergio Durigan Junior <sergiodj@redhat.com>
 3 | AuthorDate: 2019-10-30 13:58:29 -0400
 4 | Commit:     Sergio Durigan Junior <sergiodj@redhat.com>
 5 | CommitDate: 2019-11-01 11:53:06 -0400
 6 | 
 7 > Make sure we have a valid target on top when pushing a new target
 8 | 
 9 | Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
10 | 
11 | A segfault can happen in a specific scenario when using TUI + a
12 | corefile, as explained in the bug mentioned above.  The problem

I'd rename this to "Fix crash with core + TUI + run"

The current subject is talking in terms of the fix, but I think the fix isn't the right one.  More below.


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/target.c 
File gdb/target.c:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/target.c@574 
PS1, Line 574: 
559 | target_stack::push (target_ops *t)
    | ...
569 |   m_stack[stratum] = t;
570 | 
571 |   /* And close the previous one, if it exists.  */
572 |   if (prev != nullptr)
573 |     target_close (prev);
574 > 
575 |   if (m_top < stratum)
576 |     m_top = stratum;
577 | }
578 | 
579 | /* See target.h.  */

This isn't the right fix.  The reason we're clearing m_stack[stratum] before calling target_close is so that target method calls within the target_close implementation don't reach the target.

There's a comment about it at the end of target_stack::unpush(target_ops *):

>  /* Finally close the target.  Note we do this after unchaining, so
>     any target method calls from within the target_close
>     implementation don't end up in T anymore.  */
>  target_close (t);

With your change, we're now reaching the target_close implementation with the new target pushed on the stack, so if the target_close implementation calls any target method, it reaches the new target!  I.e., target_shortname inside the target_close call for the core target is returning the shortname of the native target.  Imagine if we end up trying to read memory, for example.

The real problem is that we're clearing m_stack[stratum], but forget to adjust m_top.  target_stack::unpush does exactly that:

>  /* Unchain the target.  */
>  m_stack[stratum] = NULL;
>
>  if (m_top == stratum)
>    m_top = t->beneath ()->stratum ();
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>  /* Finally close the target.  Note we do this after unchaining, so
>     any target method calls from within the target_close
>     implementation don't end up in T anymore.  */
>  target_close (t);

... so the right fix is to just call unpush, like so:

   if (m_stack[stratum] != NULL)
-    {
-      target_ops *prev = m_stack[stratum];
-      m_stack[stratum] = NULL;
-      target_close (prev);
-    }
+    unpush (m_stack[stratum]);


https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp 
File gdb/testsuite/gdb.tui/segfault-corefile-run.exp:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483/1/gdb/testsuite/gdb.tui/segfault-corefile-run.exp@1 
PS1, Line 1: 
1 > # Copyright 2019 Free Software Foundation, Inc.
2 | 
3 | # This program is free software; you can redistribute it and/or modify
4 | # it under the terms of the GNU General Public License as published by
5 | # the Free Software Foundation; either version 3 of the License, or
6 | # (at your option) any later version.

I don't know how to comment on a file name, so I'm commenting here.

I'd rather rename the file to "corefile-run.exp".  I.e., name it for what it tests, leave the bug description to the comments, not the file name.



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Tue, 05 Nov 2019 23:52:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Sergio Durigan Junior (Code Review)]

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................

Fix crash with core + TUI + run

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117

A segfault can happen in a specific scenario when using TUI + a
corefile, as explained in the bug mentioned above.  The problem
happens when opening a corefile on GDB:

  $ gdb ./core program

entering TUI (C-x a), and then issuing a "run" command.  GDB segfaults
with the following stack trace:

  (top-gdb) bt
  #0  0x00000000004cd5da in target_ops::shortname (this=0x0) at ../../binutils-gdb/gdb/target.h:449
  #1  0x0000000000ac08fb in target_shortname () at ../../binutils-gdb/gdb/target.h:1323
  #2  0x0000000000ac09ae in tui_locator_window::make_status_line[abi:cxx11]() const (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:86
  #3  0x0000000000ac1043 in tui_locator_window::rerender (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:231
  #4  0x0000000000ac1632 in tui_show_locator_content () at ../../binutils-gdb/gdb/tui/tui-stack.c:369
  #5  0x0000000000ac63b6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at ../../binutils-gdb/gdb/tui/tui.c:321
  #6  0x0000000000aaf9be in tui_inferior_exit (inf=0x2d446a0) at ../../binutils-gdb/gdb/tui/tui-hooks.c:181
  #7  0x000000000044cddf in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&) (__functor=..., __args#0=@0x7fffffffd650: 0x2d446a0)
      at /usr/include/c++/9/bits/std_function.h:300
  #8  0x0000000000757db9 in std::function<void (inferior*)>::operator()(inferior*) const (this=0x2cf3168, __args#0=0x2d446a0) at /usr/include/c++/9/bits/std_function.h:690
  #9  0x0000000000757876 in gdb::observers::observable<inferior*>::notify (this=0x23de0c0 <gdb::observers::inferior_exit>, args#0=0x2d446a0)
      at ../../binutils-gdb/gdb/gdbsupport/observable.h:106
  #10 0x000000000075532d in exit_inferior_1 (inftoex=0x2d446a0, silent=1) at ../../binutils-gdb/gdb/inferior.c:191
  #11 0x0000000000755460 in exit_inferior_silent (inf=0x2d446a0) at ../../binutils-gdb/gdb/inferior.c:234
  #12 0x000000000059f47c in core_target::close (this=0x2d68590) at ../../binutils-gdb/gdb/corelow.c:265
  #13 0x0000000000a7688c in target_close (targ=0x2d68590) at ../../binutils-gdb/gdb/target.c:3293
  #14 0x0000000000a63d74 in target_stack::push (this=0x23e1800 <g_target_stack>, t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:568
  #15 0x0000000000a63dbf in push_target (t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:583
  #16 0x0000000000748088 in inf_ptrace_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/inf-ptrace.c:128
  #17 0x0000000000795ccb in linux_nat_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/linux-nat.c:1094
  #18 0x000000000074eae9 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../binutils-gdb/gdb/infcmd.c:639
  ...

The problem happens because 'tui_locator_window::make_status_line'
needs the value of 'target_shortname' in order to update the status
line.  'target_shortname' is a macro which expands to:

  #define	target_shortname	(current_top_target ()->shortname ())

and, in our scenario, 'current_top_target ()' returns NULL, which
obviously causes a segfault.  But why does it return NULL, since,
according to its comment on target.h, it should never do that?

What is happening is that we're being caught in the middle of a
"target switch".  We had the 'core_target' on top, because we were
inspecting a corefile, but when the user decided to invoke "run" GDB
had to actually create the inferior, which ends up detecting that we
have a target already, and tries to close it (from target.c):

  /* See target.h.  */

  void
  target_stack::push (target_ops *t)
  {
    /* If there's already a target at this stratum, remove it.  */
    strata stratum = t->stratum ();

    if (m_stack[stratum] != NULL)
      {
	target_ops *prev = m_stack[stratum];
	m_stack[stratum] = NULL;
	target_close (prev); // <-- here
      }
  ...

When the current target ('core_target') is being closed, it checks for
possible observers registered with it and calls them.  TUI is one of
those observers, it gets called, tries to update the status line, and
GDB crashes.

The real problem is that we are calling 'target_close' too soon,
before we actually have the new target on top of the stack.
Interestingly, this scenario is covered in 'target_stack::unpush', but
for some reason we are not calling it here.  The fix, therefore, is to
call 'unpush' if there's a target on the stack.

This patch has been tested on the Buildbot and no regressions have
been found.  I'm also submitting a testcase for it.

gdb/ChangeLog:
2019-11-08  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* target.c (target_stack::push): Call 'unpush' if there's a
	target on top of the stack.

gdb/testsuite/ChangeLog:
2019-11-08  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* gdb.tui/corefile-run.exp: New file.

Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
---
M gdb/target.c
A gdb/testsuite/gdb.tui/corefile-run.exp
2 files changed, 62 insertions(+), 5 deletions(-)



diff --git a/gdb/target.c b/gdb/target.c
index 7c9befc..5a3764e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -562,11 +562,7 @@
   strata stratum = t->stratum ();
 
   if (m_stack[stratum] != NULL)
-    {
-      target_ops *prev = m_stack[stratum];
-      m_stack[stratum] = NULL;
-      target_close (prev);
-    }
+    unpush (m_stack[stratum]);
 
   /* Now add the new one.  */
   m_stack[stratum] = t;
diff --git a/gdb/testsuite/gdb.tui/corefile-run.exp b/gdb/testsuite/gdb.tui/corefile-run.exp
new file mode 100644
index 0000000..05fcc24
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/corefile-run.exp
@@ -0,0 +1,61 @@
+# Copyright 2019 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 whether we can load a corefile, enable TUI and then invoke
+# "run" without having a segfault.
+#
+# Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+set core "${testfile}.core"
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Only run on native boards.
+if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+    untested "not supported"
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+if { ![gdb_gcore_cmd "$core" "save a corefile"] } {
+    untested "could not generate a corefile"
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+}
+
+set text [Term::get_all_lines]
+gdb_assert {![string match "No Source Available" $text]} \
+    "initial source listing"
+
+Term::command "core-file $core"
+Term::check_contents "load corefile" "21 *return 0"
+
+Term::command "run"
+Term::check_contents "run until the end" \
+    "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: newpatchset

[review] Fix crash with core + TUI + run

[Sergio Durigan Junior (Code Review)]

Sergio Durigan Junior has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 1:

(3 comments)

On Tuesday, November 05 2019, Pedro Alves wrote:

> --- /dev/null
> +++ /COMMIT_MSG
> @@ -1,0 +1,16 @@ 
> +Parent:     e48f6033 (Move check for strerror_r to common.m4 where it belongs)
> +Author:     Sergio Durigan Junior <sergiodj@redhat.com>
> +AuthorDate: 2019-10-30 13:58:29 -0400
> +Commit:     Sergio Durigan Junior <sergiodj@redhat.com>
> +CommitDate: 2019-11-01 11:53:06 -0400
> +
> +Make sure we have a valid target on top when pushing a new target

PS1, Line 7:


Thanks for the review.  I changed the subject as suggested.

> +
> +Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
> +
> +A segfault can happen in a specific scenario when using TUI + a
> +corefile, as explained in the bug mentioned above.  The problem
> +happens when opening a corefile on GDB:
> +
> +  $ gdb ./core program
> +
> --- gdb/target.c
> +++ gdb/target.c
> @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
> -    }
> +    prev = m_stack[stratum];
>  
>    /* Now add the new one.  */
>    m_stack[stratum] = t;
>  
> +  /* And close the previous one, if it exists.  */
> +  if (prev != nullptr)
> +    target_close (prev);
> +

PS1, Line 574:

>>  /* Finally close the target.  Note we do this after unchaining, so
>>     any target method calls from within the target_close
>>     implementation don't end up in T anymore.  */
>>  target_close (t);

>>  /* Unchain the target.  */
>>  m_stack[stratum] = NULL;
>>
>>  if (m_top == stratum)
>>    m_top = t->beneath ()->stratum ();

>>
>>  /* Finally close the target.  Note we do this after unchaining, so
>>     any target method calls from within the target_close
>>     implementation don't end up in T anymore.  */
>>  target_close (t);


Interesting.  While hacking, I wasn't sure if I should call 'unpush'
directly (even wrote about that in the commit message), but in the end
decided to just reorder the call to 'target_close'.  It's interesting
that I read the comment on 'unpush' regarding calling 'target_close'
after unchaining...  Anyway, thanks for pointing this out.  I updated
the patch.

>    if (m_top < stratum)
>      m_top = stratum;
>  }
>  
>  /* See target.h.  */
>  
>  void
>  push_target (struct target_ops *t)
>  {
> --- /dev/null
> +++ gdb/testsuite/gdb.tui/segfault-corefile-run.exp
> @@ -1,0 +1,10 @@ 
> +# Copyright 2019 Free Software Foundation, Inc.

PS1, Line 1:


Fair enough; renamed.  Thanks.

> +
> +# 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

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Fri, 08 Nov 2019 21:26:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

(1 comment)

> --- gdb/target.c
> +++ gdb/target.c
> @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
> -    }
> +    prev = m_stack[stratum];
>  
>    /* Now add the new one.  */
>    m_stack[stratum] = t;
>  
> +  /* And close the previous one, if it exists.  */
> +  if (prev != nullptr)
> +    target_close (prev);
> +

PS1, Line 574:

Soundcheck.  1... 2... 3...

>    if (m_top < stratum)
>      m_top = stratum;
>  }
>  
>  /* See target.h.  */
>  
>  void
>  push_target (struct target_ops *t)
>  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Fri, 08 Nov 2019 23:13:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

(1 comment)

> --- gdb/target.c
> +++ gdb/target.c
> @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
> -    }
> +    prev = m_stack[stratum];
>  
>    /* Now add the new one.  */
>    m_stack[stratum] = t;
>  
> +  /* And close the previous one, if it exists.  */
> +  if (prev != nullptr)
> +    target_close (prev);
> +

PS1, Line 574:

> Soundcheck.  1... 2... 3...

Now with a quote.

>    if (m_top < stratum)
>      m_top = stratum;
>  }
>  
>  /* See target.h.  */
>  
>  void
>  push_target (struct target_ops *t)
>  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Fri, 08 Nov 2019 23:14:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

(1 comment)

> --- gdb/target.c
> +++ gdb/target.c
> @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
> -    }
> +    prev = m_stack[stratum];
>  
>    /* Now add the new one.  */
>    m_stack[stratum] = t;
>  
> +  /* And close the previous one, if it exists.  */
> +  if (prev != nullptr)
> +    target_close (prev);
> +

PS1, Line 574:

> Now with a quote.

Quote^2.

>    if (m_top < stratum)
>      m_top = stratum;
>  }
>  
>  /* See target.h.  */
>  
>  void
>  push_target (struct target_ops *t)
>  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Fri, 08 Nov 2019 23:21:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Simon Marchi (Code Review)]

Simon Marchi has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

(1 comment)

| --- gdb/target.c
| +++ gdb/target.c
| @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
| -    }
| +    prev = m_stack[stratum];
|  
|    /* Now add the new one.  */
|    m_stack[stratum] = t;
|  
| +  /* And close the previous one, if it exists.  */
| +  if (prev != nullptr)
| +    target_close (prev);
| +

PS1, Line 574:

> > Now with a quote.
> 
> Quote^2.

Testing again.

|    if (m_top < stratum)
|      m_top = stratum;
|  }
|  
|  /* See target.h.  */
|  
|  void
|  push_target (struct target_ops *t)
|  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Sat, 09 Nov 2019 00:05:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Simon Marchi (2) (Code Review)]

Simon Marchi (2) has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

On 2019-11-08 7:05 p.m., Simon Marchi (Code Review) wrote:

>>> Now with a quote.
>>
>> Quote^2.

Testing the reply by email now.


-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Sat, 09 Nov 2019 00:11:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Pedro Alves (Code Review)]

Pedro Alves has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

This is OK, but please adjust the commit log comment, as pointed out below, before pushing.

Thanks for fixing this, and for the testcase.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +74,20 @@ have a target already, and tries to close it (from target.c):
| +	target_close (prev); // <-- here
| +      }
| +  ...
| +
| +When the current target ('core_target') is being closed, it checks for
| +possible observers registered with it and calls them.  TUI is one of
| +those observers, it gets called, tries to update the status line, and
| +GDB crashes.
| +
| +The real problem is that we are calling 'target_close' too soon,
| +before we actually have the new target on top of the stack.

PS2, Line 84:

No, as I mentioned in a previous comment, the real problem is that
we're clearing m_stack[stratum], but forget to adjust m_top.
target_stack::unpush does that, but here I forgot to do it.
Please adjust the comment.

| +Interestingly, this scenario is covered in 'target_stack::unpush', but
| +for some reason we are not calling it here.  The fix, therefore, is to
| +call 'unpush' if there's a target on the stack.
| +
| +This patch has been tested on the Buildbot and no regressions have
| +been found.  I'm also submitting a testcase for it.
| +
| +gdb/ChangeLog:
| +2019-11-08  Sergio Durigan Junior  <sergiodj@redhat.com>

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 12 Nov 2019 12:57:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[review v2] Fix crash with core + TUI + run

[Sergio Durigan Junior (Code Review)]

Sergio Durigan Junior has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................


Patch Set 2:

(2 comments)

Thanks for the review.  I updated the commit message and will push the patch.

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +74,20 @@ have a target already, and tries to close it (from target.c):
| +	target_close (prev); // <-- here
| +      }
| +  ...
| +
| +When the current target ('core_target') is being closed, it checks for
| +possible observers registered with it and calls them.  TUI is one of
| +those observers, it gets called, tries to update the status line, and
| +GDB crashes.
| +
| +The real problem is that we are calling 'target_close' too soon,
| +before we actually have the new target on top of the stack.

PS2, Line 84:

Done.

| +Interestingly, this scenario is covered in 'target_stack::unpush', but
| +for some reason we are not calling it here.  The fix, therefore, is to
| +call 'unpush' if there's a target on the stack.
| +
| +This patch has been tested on the Buildbot and no regressions have
| +been found.  I'm also submitting a testcase for it.
| +
| +gdb/ChangeLog:
| +2019-11-08  Sergio Durigan Junior  <sergiodj@redhat.com>
| --- gdb/target.c
| +++ gdb/target.c
| @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
| -    }
| +    prev = m_stack[stratum];
|  
|    /* Now add the new one.  */
|    m_stack[stratum] = t;
|  
| +  /* And close the previous one, if it exists.  */
| +  if (prev != nullptr)
| +    target_close (prev);
| +

PS1, Line 574:

Done

|    if (m_top < stratum)
|      m_top = stratum;
|  }
|  
|  /* See target.h.  */
|  
|  void
|  push_target (struct target_ops *t)
|  {

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 2
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-Comment-Date: Tue, 19 Nov 2019 00:11:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>
Comment-In-Reply-To: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-MessageType: comment

[pushed] Fix crash with core + TUI + run

[Sourceware to Gerrit sync (Code Review)]

Sourceware to Gerrit sync has submitted this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................

Fix crash with core + TUI + run

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117

A segfault can happen in a specific scenario when using TUI + a
corefile, as explained in the bug mentioned above.  The problem
happens when opening a corefile on GDB:

  $ gdb ./core program

entering TUI (C-x a), and then issuing a "run" command.  GDB segfaults
with the following stack trace:

  (top-gdb) bt
  #0  0x00000000004cd5da in target_ops::shortname (this=0x0) at ../../binutils-gdb/gdb/target.h:449
  #1  0x0000000000ac08fb in target_shortname () at ../../binutils-gdb/gdb/target.h:1323
  #2  0x0000000000ac09ae in tui_locator_window::make_status_line[abi:cxx11]() const (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:86
  #3  0x0000000000ac1043 in tui_locator_window::rerender (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:231
  #4  0x0000000000ac1632 in tui_show_locator_content () at ../../binutils-gdb/gdb/tui/tui-stack.c:369
  #5  0x0000000000ac63b6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at ../../binutils-gdb/gdb/tui/tui.c:321
  #6  0x0000000000aaf9be in tui_inferior_exit (inf=0x2d446a0) at ../../binutils-gdb/gdb/tui/tui-hooks.c:181
  #7  0x000000000044cddf in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&) (__functor=..., __args#0=@0x7fffffffd650: 0x2d446a0)
      at /usr/include/c++/9/bits/std_function.h:300
  #8  0x0000000000757db9 in std::function<void (inferior*)>::operator()(inferior*) const (this=0x2cf3168, __args#0=0x2d446a0) at /usr/include/c++/9/bits/std_function.h:690
  #9  0x0000000000757876 in gdb::observers::observable<inferior*>::notify (this=0x23de0c0 <gdb::observers::inferior_exit>, args#0=0x2d446a0)
      at ../../binutils-gdb/gdb/gdbsupport/observable.h:106
  #10 0x000000000075532d in exit_inferior_1 (inftoex=0x2d446a0, silent=1) at ../../binutils-gdb/gdb/inferior.c:191
  #11 0x0000000000755460 in exit_inferior_silent (inf=0x2d446a0) at ../../binutils-gdb/gdb/inferior.c:234
  #12 0x000000000059f47c in core_target::close (this=0x2d68590) at ../../binutils-gdb/gdb/corelow.c:265
  #13 0x0000000000a7688c in target_close (targ=0x2d68590) at ../../binutils-gdb/gdb/target.c:3293
  #14 0x0000000000a63d74 in target_stack::push (this=0x23e1800 <g_target_stack>, t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:568
  #15 0x0000000000a63dbf in push_target (t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:583
  #16 0x0000000000748088 in inf_ptrace_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/inf-ptrace.c:128
  #17 0x0000000000795ccb in linux_nat_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/linux-nat.c:1094
  #18 0x000000000074eae9 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../binutils-gdb/gdb/infcmd.c:639
  ...

The problem happens because 'tui_locator_window::make_status_line'
needs the value of 'target_shortname' in order to update the status
line.  'target_shortname' is a macro which expands to:

  #define	target_shortname	(current_top_target ()->shortname ())

and, in our scenario, 'current_top_target ()' returns NULL, which
obviously causes a segfault.  But why does it return NULL, since,
according to its comment on target.h, it should never do that?

What is happening is that we're being caught in the middle of a
"target switch".  We had the 'core_target' on top, because we were
inspecting a corefile, but when the user decided to invoke "run" GDB
had to actually create the inferior, which ends up detecting that we
have a target already, and tries to close it (from target.c):

  /* See target.h.  */

  void
  target_stack::push (target_ops *t)
  {
    /* If there's already a target at this stratum, remove it.  */
    strata stratum = t->stratum ();

    if (m_stack[stratum] != NULL)
      {
	target_ops *prev = m_stack[stratum];
	m_stack[stratum] = NULL;
	target_close (prev); // <-- here
      }
  ...

When the current target ('core_target') is being closed, it checks for
possible observers registered with it and calls them.  TUI is one of
those observers, it gets called, tries to update the status line, and
GDB crashes.

The real problem is that we are clearing 'm_stack[stratum]', but
forgetting to adjust 'm_top'.  Interestingly, this scenario is covered
in 'target_stack::unpush', but Pedro said he forgot to call it here..
The fix, therefore, is to call '::unpush' if there's a target on the
stack.

This patch has been tested on the Buildbot and no regressions have
been found.  I'm also submitting a testcase for it.

gdb/ChangeLog:
2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* target.c (target_stack::push): Call 'unpush' if there's a
	target on top of the stack.

gdb/testsuite/ChangeLog:
2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* gdb.tui/corefile-run.exp: New file.

Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
---
M gdb/ChangeLog
M gdb/target.c
M gdb/testsuite/ChangeLog
A gdb/testsuite/gdb.tui/corefile-run.exp
4 files changed, 74 insertions(+), 5 deletions(-)


diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0dfc96a..80ec8f3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+	* target.c (target_stack::push): Call 'unpush' if there's a
+	target on top of the stack.
+
 2019-11-18  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* python/py-block.c (blpy_dealloc): Call tp_free.
diff --git a/gdb/target.c b/gdb/target.c
index 7c9befc..5a3764e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -562,11 +562,7 @@
   strata stratum = t->stratum ();
 
   if (m_stack[stratum] != NULL)
-    {
-      target_ops *prev = m_stack[stratum];
-      m_stack[stratum] = NULL;
-      target_close (prev);
-    }
+    unpush (m_stack[stratum]);
 
   /* Now add the new one.  */
   m_stack[stratum] = t;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3a4d229..de8712a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+	* gdb.tui/corefile-run.exp: New file.
+
 2019-11-14  Tom Tromey  <tromey@adacore.com>
 
 	* gdb.base/gdbvars.exp (test_convenience_variables): Add
diff --git a/gdb/testsuite/gdb.tui/corefile-run.exp b/gdb/testsuite/gdb.tui/corefile-run.exp
new file mode 100644
index 0000000..05fcc24
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/corefile-run.exp
@@ -0,0 +1,61 @@
+# Copyright 2019 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 whether we can load a corefile, enable TUI and then invoke
+# "run" without having a segfault.
+#
+# Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+set core "${testfile}.core"
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Only run on native boards.
+if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+    untested "not supported"
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+if { ![gdb_gcore_cmd "$core" "save a corefile"] } {
+    untested "could not generate a corefile"
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+}
+
+set text [Term::get_all_lines]
+gdb_assert {![string match "No Source Available" $text]} \
+    "initial source listing"
+
+Term::command "core-file $core"
+Term::check_contents "load corefile" "21 *return 0"
+
+Term::command "run"
+Term::check_contents "run until the end" \
+    "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 3
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: merged

[pushed] Fix crash with core + TUI + run

[Sourceware to Gerrit sync (Code Review)]

The original change was created by Sergio Durigan Junior.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................

Fix crash with core + TUI + run

Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117

A segfault can happen in a specific scenario when using TUI + a
corefile, as explained in the bug mentioned above.  The problem
happens when opening a corefile on GDB:

  $ gdb ./core program

entering TUI (C-x a), and then issuing a "run" command.  GDB segfaults
with the following stack trace:

  (top-gdb) bt
  #0  0x00000000004cd5da in target_ops::shortname (this=0x0) at ../../binutils-gdb/gdb/target.h:449
  #1  0x0000000000ac08fb in target_shortname () at ../../binutils-gdb/gdb/target.h:1323
  #2  0x0000000000ac09ae in tui_locator_window::make_status_line[abi:cxx11]() const (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:86
  #3  0x0000000000ac1043 in tui_locator_window::rerender (this=0x23e1fa0 <_locator>) at ../../binutils-gdb/gdb/tui/tui-stack.c:231
  #4  0x0000000000ac1632 in tui_show_locator_content () at ../../binutils-gdb/gdb/tui/tui-stack.c:369
  #5  0x0000000000ac63b6 in tui_set_key_mode (mode=TUI_COMMAND_MODE) at ../../binutils-gdb/gdb/tui/tui.c:321
  #6  0x0000000000aaf9be in tui_inferior_exit (inf=0x2d446a0) at ../../binutils-gdb/gdb/tui/tui-hooks.c:181
  #7  0x000000000044cddf in std::_Function_handler<void (inferior*), void (*)(inferior*)>::_M_invoke(std::_Any_data const&, inferior*&&) (__functor=..., __args#0=@0x7fffffffd650: 0x2d446a0)
      at /usr/include/c++/9/bits/std_function.h:300
  #8  0x0000000000757db9 in std::function<void (inferior*)>::operator()(inferior*) const (this=0x2cf3168, __args#0=0x2d446a0) at /usr/include/c++/9/bits/std_function.h:690
  #9  0x0000000000757876 in gdb::observers::observable<inferior*>::notify (this=0x23de0c0 <gdb::observers::inferior_exit>, args#0=0x2d446a0)
      at ../../binutils-gdb/gdb/gdbsupport/observable.h:106
  #10 0x000000000075532d in exit_inferior_1 (inftoex=0x2d446a0, silent=1) at ../../binutils-gdb/gdb/inferior.c:191
  #11 0x0000000000755460 in exit_inferior_silent (inf=0x2d446a0) at ../../binutils-gdb/gdb/inferior.c:234
  #12 0x000000000059f47c in core_target::close (this=0x2d68590) at ../../binutils-gdb/gdb/corelow.c:265
  #13 0x0000000000a7688c in target_close (targ=0x2d68590) at ../../binutils-gdb/gdb/target.c:3293
  #14 0x0000000000a63d74 in target_stack::push (this=0x23e1800 <g_target_stack>, t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:568
  #15 0x0000000000a63dbf in push_target (t=0x23c38c8 <the_amd64_linux_nat_target>) at ../../binutils-gdb/gdb/target.c:583
  #16 0x0000000000748088 in inf_ptrace_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/inf-ptrace.c:128
  #17 0x0000000000795ccb in linux_nat_target::create_inferior (this=0x23c38c8 <the_amd64_linux_nat_target>, exec_file=0x2d58d30 "/usr/bin/cat", allargs="", env=0x25f12b0, from_tty=1)
      at ../../binutils-gdb/gdb/linux-nat.c:1094
  #18 0x000000000074eae9 in run_command_1 (args=0x0, from_tty=1, run_how=RUN_NORMAL) at ../../binutils-gdb/gdb/infcmd.c:639
  ...

The problem happens because 'tui_locator_window::make_status_line'
needs the value of 'target_shortname' in order to update the status
line.  'target_shortname' is a macro which expands to:

  #define	target_shortname	(current_top_target ()->shortname ())

and, in our scenario, 'current_top_target ()' returns NULL, which
obviously causes a segfault.  But why does it return NULL, since,
according to its comment on target.h, it should never do that?

What is happening is that we're being caught in the middle of a
"target switch".  We had the 'core_target' on top, because we were
inspecting a corefile, but when the user decided to invoke "run" GDB
had to actually create the inferior, which ends up detecting that we
have a target already, and tries to close it (from target.c):

  /* See target.h.  */

  void
  target_stack::push (target_ops *t)
  {
    /* If there's already a target at this stratum, remove it.  */
    strata stratum = t->stratum ();

    if (m_stack[stratum] != NULL)
      {
	target_ops *prev = m_stack[stratum];
	m_stack[stratum] = NULL;
	target_close (prev); // <-- here
      }
  ...

When the current target ('core_target') is being closed, it checks for
possible observers registered with it and calls them.  TUI is one of
those observers, it gets called, tries to update the status line, and
GDB crashes.

The real problem is that we are clearing 'm_stack[stratum]', but
forgetting to adjust 'm_top'.  Interestingly, this scenario is covered
in 'target_stack::unpush', but Pedro said he forgot to call it here..
The fix, therefore, is to call '::unpush' if there's a target on the
stack.

This patch has been tested on the Buildbot and no regressions have
been found.  I'm also submitting a testcase for it.

gdb/ChangeLog:
2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
	    Pedro Alves  <palves@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* target.c (target_stack::push): Call 'unpush' if there's a
	target on top of the stack.

gdb/testsuite/ChangeLog:
2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>

	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
	* gdb.tui/corefile-run.exp: New file.

Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
---
M gdb/ChangeLog
M gdb/target.c
M gdb/testsuite/ChangeLog
A gdb/testsuite/gdb.tui/corefile-run.exp
4 files changed, 74 insertions(+), 5 deletions(-)



diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0dfc96a..80ec8f3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
+	    Pedro Alves  <palves@redhat.com>
+
+	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+	* target.c (target_stack::push): Call 'unpush' if there's a
+	target on top of the stack.
+
 2019-11-18  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
 
 	* python/py-block.c (blpy_dealloc): Call tp_free.
diff --git a/gdb/target.c b/gdb/target.c
index 7c9befc..5a3764e 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -562,11 +562,7 @@
   strata stratum = t->stratum ();
 
   if (m_stack[stratum] != NULL)
-    {
-      target_ops *prev = m_stack[stratum];
-      m_stack[stratum] = NULL;
-      target_close (prev);
-    }
+    unpush (m_stack[stratum]);
 
   /* Now add the new one.  */
   m_stack[stratum] = t;
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3a4d229..de8712a 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2019-11-18  Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+	* gdb.tui/corefile-run.exp: New file.
+
 2019-11-14  Tom Tromey  <tromey@adacore.com>
 
 	* gdb.base/gdbvars.exp (test_convenience_variables): Add
diff --git a/gdb/testsuite/gdb.tui/corefile-run.exp b/gdb/testsuite/gdb.tui/corefile-run.exp
new file mode 100644
index 0000000..05fcc24
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/corefile-run.exp
@@ -0,0 +1,61 @@
+# Copyright 2019 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 whether we can load a corefile, enable TUI and then invoke
+# "run" without having a segfault.
+#
+# Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
+
+load_lib "tuiterm.exp"
+
+standard_testfile tui-layout.c
+
+set core "${testfile}.core"
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
+    return -1
+}
+
+# Only run on native boards.
+if { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
+    untested "not supported"
+    return
+}
+
+if { ![runto_main] } {
+    untested "could not run to main"
+    return -1
+}
+
+if { ![gdb_gcore_cmd "$core" "save a corefile"] } {
+    untested "could not generate a corefile"
+    return -1
+}
+
+Term::clean_restart 24 80 $testfile
+if {![Term::enter_tui]} {
+    unsupported "TUI not supported"
+}
+
+set text [Term::get_all_lines]
+gdb_assert {![string match "No Source Available" $text]} \
+    "initial source listing"
+
+Term::command "core-file $core"
+Term::check_contents "load corefile" "21 *return 0"
+
+Term::command "run"
+Term::check_contents "run until the end" \
+    "\\\[Inferior $decimal \\\(process $decimal\\\) exited normally\\]"

-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 3
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-CC: Simon Marchi <simon.marchi@polymtl.ca>
Gerrit-MessageType: newpatchset