Re: [PATCH 3/3] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new (PR 23368)

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-10-20 11:13 a.m., Simon Marchi wrote:
> On 2018-10-17 1:04 p.m., Pedro Alves wrote:
>> On 10/16/2018 04:38 AM, Simon Marchi wrote:
>>> Here's a summary of PR 23368:
>>>
>>>   #include <unistd.h>
>>>   int main (void)
>>>   {
>>>     char *exec_args[] = { "/bin/ls", NULL };
>>>     execve (exec_args[0], exec_args, NULL);
>>>   }
>>>
>>> $ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
>>> ...
>>> [1]  + 13146 suspended (tty output)  gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
>>> $
>>>
>>> Here's what happens: when the inferior execs with "follow-exec-mode
>>> new", we first "mourn" it before creating the new one.  This ends up
>>> calling inflow_inferior_exit, which sets the per-inferior terminal state
>>> to "is_ours":
>>>
>>>   inf->terminal_state = target_terminal_state::is_ours;
>>>
>>> At this point, the inferior's terminal_state is is_ours, while the
>>> "reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the
>>> terminal).
>>>
>>> Later, we continue processing the exec inferior event and decide we want
>>> to stop (because of the "catch exec") and call target_terminal::ours to
>>> make sure we own the terminal.  However, we don't actually go to the
>>> target backend to change the settings, because the core thinks that no
>>> inferior owns the terminal (inf->terminal_state is
>>> target_terminal_state::is_ours, as checked in
>>> target_terminal_is_ours_kind, for both inferiors). 
>>                                 ^^^^^^^^^^^^^^^^^^
>>
>> I think the "for both inferiors" part is what's dubious here.
>>
>> The process lives on in the new inferior, but we lost its
>> terminal settings!  Seems to me that they should be migrated
>> from the old inferior to the new one.  And then the problem
>> sorts itself out, because then the new inferior will have
>> target_terminal_state::is_inferior state.
> 
> Yeah that makes sense.  This crossed my mind when I look at the issue,
> but for some reason I didn't went that route.  But now that you say it,
> it appears obvious that this should be the fix.
> 
> I just tried copying the inferior::terminal_state value from the old
> inferior to the new inferior, and it fixes the problem.  But
> actually, we should also be transferring all of the struct terminal_info
> associated to the inferior, is that right?

This would be the updated patch (testing on the buildbot at the moment).


From c54c1dc3f124aef65c8a1daf042ded2b6b4a8a46 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sat, 20 Oct 2018 10:57:33 -0400
Subject: [PATCH] Avoid GDB SIGTTOU on catch exec + set follow-exec-mode new
 (PR 23368)

Here's a summary of PR 23368:

  #include <unistd.h>
  int main (void)
  {
    char *exec_args[] = { "/bin/ls", NULL };
    execve (exec_args[0], exec_args, NULL);
  }

$ gdb -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
...
[1]  + 13146 suspended (tty output)  gdb -q -nx t -ex "catch exec" -ex "set follow-exec-mode new" -ex run
$

Here's what happens: when the inferior execs with "follow-exec-mode
new", we first "mourn" it before creating the new one.  This ends up
calling inflow_inferior_exit, which sets the per-inferior terminal state
to "is_ours":

  inf->terminal_state = target_terminal_state::is_ours;

At this point, the inferior's terminal_state is is_ours, while the
"reality", tracked by gdb_tty_state, is is_inferior (GDB doesn't own the
terminal).

Later, we continue processing the exec inferior event and decide we want
to stop (because of the "catch exec") and call target_terminal::ours to
make sure we own the terminal.  However, we don't actually go to the
target backend to change the settings, because the core thinks that no
inferior owns the terminal (inf->terminal_state is
target_terminal_state::is_ours, as checked in
target_terminal_is_ours_kind, for both inferiors).  When something in
readline tries to mess with the terminal settings, it generates a
SIGTTOU.

This patch fixes this by tranferring the state of the terminal from the
old inferior to the new inferior.

gdb/ChangeLog:

	* infrun.c (follow_exec): In the follow_exec_mode_new case,
	transfer terminal state from old new new inferior.
---
 gdb/infrun.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 72e249617670..85a5200ddf4b 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -1189,12 +1189,13 @@ follow_exec (ptid_t ptid, char *exec_file_target)
       /* The user wants to keep the old inferior and program spaces
 	 around.  Create a new fresh one, and switch to it.  */

-      /* Do exit processing for the original inferior before adding
-	 the new inferior so we don't have two active inferiors with
-	 the same ptid, which can confuse find_inferior_ptid.  */
+      /* Do exit processing for the original inferior before setting the new
+	 inferior's pid.  Having two inferiors with the same pid would confuse
+	 find_inferior_p(t)id.  */
+      inf = add_inferior_with_spaces ();
+      copy_terminal_info (inf, current_inferior ());
       exit_inferior_silent (current_inferior ());

-      inf = add_inferior_with_spaces ();
       inf->pid = pid;
       target_follow_exec (inf, exec_file_target);

-- 
2.19.1