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

["Pedro Alves (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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