[3/3] 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 manages to fix this particular case by calling
target_terminal::ours() in inflow_inferior_exit.  This makes so that
inflow actually changes the terminal settings so that GDB owns it, which
avoids the SIGTTOU later.

The buildbot doesn't complain, but I am not sure this is the most
bestest way to fix this, maybe it's just papering over the actual
problem or introduces some latent bug.  In particular, I am not sure if
this is correct in case we have multiple inferiors sharing the same
terminal.  But I thought I would still submit this patch to get the ball
rolling.

gdb/ChangeLog:

	PR gdb/23368
	* inflow.c (inflow_inferior_exit): Update doc.  Call
	target_terminal::ours.
---
 gdb/inflow.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
  

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.

> When something in
> readline tries to mess with the terminal settings, it generates a
> SIGTTOU.
> 
> This patch manages to fix this particular case by calling
> target_terminal::ours() in inflow_inferior_exit.  This makes so that
> inflow actually changes the terminal settings so that GDB owns it, which
> avoids the SIGTTOU later.
> 
> The buildbot doesn't complain, but I am not sure this is the most
> bestest way to fix this, maybe it's just papering over the actual
> problem or introduces some latent bug.  In particular, I am not sure if
> this is correct in case we have multiple inferiors sharing the same
> terminal.  But I thought I would still submit this patch to get the ball
> rolling.

Yeah, I don't think this is right in the multi-inferior (or multi-target)
case.  It may happen to just work because we always stop everything
when an inferior exits.  In the exec case, if you don't catch the exec,
then we won't stop; likely it ends up working because we end up calling
target_terminal::inferior() on resume.  Still, I think this is
a hack that would likely cause trouble down the road.

> 
> gdb/ChangeLog:
> 
> 	PR gdb/23368
> 	* inflow.c (inflow_inferior_exit): Update doc.  Call
> 	target_terminal::ours.
> ---
>  gdb/inflow.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index e355f4aa9fc5..95e195167ef7 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -653,18 +653,15 @@ get_inflow_inferior_data (struct inferior *inf)
>    return info;
>  }
>  
> -/* This is a "inferior_exit" observer.  Releases the TERMINAL_INFO member
> -   of the inferior structure.  This field is private to inflow.c, and
> -   its type is opaque to the rest of GDB.  PID is the target pid of
> -   the inferior that is about to be removed from the inferior
> -   list.  */
> +/* This is a "inferior_exit" observer.  Releases the terminal info associated
> +   to INF.  */
>  
>  static void
>  inflow_inferior_exit (struct inferior *inf)
>  {
>    struct terminal_info *info;
>  
> -  inf->terminal_state = target_terminal_state::is_ours;
> +  target_terminal::ours ();
>  
>    info = (struct terminal_info *) inferior_data (inf, inflow_inferior_data);
>    if (info != NULL)

Thanks,
Pedro Alves
  

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?

> 
>> When something in
>> readline tries to mess with the terminal settings, it generates a
>> SIGTTOU.
>>
>> This patch manages to fix this particular case by calling
>> target_terminal::ours() in inflow_inferior_exit.  This makes so that
>> inflow actually changes the terminal settings so that GDB owns it, which
>> avoids the SIGTTOU later.
>>
>> The buildbot doesn't complain, but I am not sure this is the most
>> bestest way to fix this, maybe it's just papering over the actual
>> problem or introduces some latent bug.  In particular, I am not sure if
>> this is correct in case we have multiple inferiors sharing the same
>> terminal.  But I thought I would still submit this patch to get the ball
>> rolling.
> 
> Yeah, I don't think this is right in the multi-inferior (or multi-target)
> case.  It may happen to just work because we always stop everything
> when an inferior exits.  In the exec case, if you don't catch the exec,
> then we won't stop; likely it ends up working because we end up calling
> target_terminal::inferior() on resume.  Still, I think this is
> a hack that would likely cause trouble down the road.

Ok.

Thanks,

Simon