Re: [pushed+7.8] Re: [PATCH] Fix "attach" command vs user input race [Re: Regression for attach from stdin [Re: [pushed] Re: [PATCH v6 0/2] enable target-async by default]]

Pedro Alves
2014-07-30T12:38:09Z

On 07/29/2014 11:03 PM, Doug Evans wrote:
> A related issue (or the same one if one prefers):
>
> post_create_inferior does this:
>
> /* Be sure we own the terminal in case write operations are performed. */
> target_terminal_ours ();

Yeah.

As I mentioned before, when I wrote a test that sends a ctrl-c right
after "attach", which triggered the issue mentioned in the new
comment,

- installs a SIGINT handler that forwards SIGINT to the inferior.
Otherwise a Ctrl-C pressed just while waiting for the initial
stop would end up as a spurious Quit.

it tripped on a set of other races.

This call was one of them. We should actually be calling
target_terminal_ours_for_output, not target_terminal_ours, so
that stdin/ctrl-c is still redirected to the inferior. As is,
IIRC, if a ctrl-c arrives after than target_terminal_ours and before
the next target_terminal_inferior, we end up in the prompt, with
a Quit, but with the target still running.

I gave up fixing those at the time, as they're orthogonal to
sync/async.

>
> but post_create_inferior is called *after* target_post_attach
> in attach_command_post_wait:
>
> /* Take any necessary post-attaching actions for this platform. */
> target_post_attach (ptid_get_pid (inferior_ptid));
>
> post_create_inferior (&current_target, from_tty);
>
> What if target_post_attach does some writes?

Something should call target_terminal_ours_for_output before.

> Seems like it can, e.g., some of the ptrace checking stuff may print a warning.
> Plus attach_command_post_wait calls some other stuff before
> post_create_inferior that could cause some writes to the terminal.
>
> Question: Is there a specific terminal state that needs to be in
> effect when attach_command_post_wait returns?
> Or can we just call target_terminal_ours at its start?

I think we can.

> [and leave it to other code to switch back to target_terminal_inferior
> as needed - e.g. proceed calls resume which will call
> target_terminal_inferior]

Even with that, it'll still be possible to have a ctrl-c pressed after
to_attach is called, and before target_terminal_inferior is called
(and therefore set_sigint_trap is called, which sets up ctrl-c
forwarding).

When I last looked at this, it seemed to be that ideally, we should
only have a single SIGINT handler, which just records that a ctrl-c was
pressed, and then the event loop decides whether a ctrl-c is a Quit or
whether it should be forwarded to the inferior, depending on whether the
target is running on the foreground, or not.

--
Thanks,
Pedro Alves