Hi Takashi,
On Wed, 7 May 2025, Takashi Yano wrote:
> ... and restore it when app exits. The commit 0bfd91d57863 has a bug
> that the console mode is stored into the shared memory when both:
> (1) cygwin process is started from non-cygwin process.
> (2) cygwin process started from non-cygwin process exits.
> (1) is intended, but (2) is not. Due to (2), the stored console mode
> is unexpectedly broken when the cygwin process exits. Then the mode
> restored will be not as expected. This causes undesired console mode
> in the use case that cygwin and non-cygwin apps are mixed.
>
> With this patch, the console mode will stored only in the case (1).
> This is done by putting the code, which stores the console mode, into
> fhandler_console::open() rather than fhandler_console::set_input_mode()
> and fhandler_console::set_output_mode().
>
> Fixes: 0bfd91d57863 ("Cygwin: console: tty::restore really restores the
> previous mode")
> Reported-by: Johannes Schindelin <[email protected]>
> Signed-off-by: Takashi Yano <[email protected]>
Thank you! I can confirm that this re-fixes the problem fixed in
3312f2d21f (Cygwin: console: Redesign mode set strategy on close().,
2025-03-03), which was in response to a report I received in
https://github.com/microsoft/git/issues/730.
I also asked the reporter of the bug you fixed in 114cbda779 (Cygwin:
console: tty::restore really restores the previous mode, 2025-03-21)
(which was the cause for the regression I reported) to verify that _their_
bug did not reappear, see
https://github.com/msys2/msys2-runtime/issues/268#issuecomment-2858071841
Finally, I spent quite a few hours to write an automated test that
imitates my reproducer, and integrated it into Git for Windows' CI builds:
https://github.com/git-for-windows/msys2-runtime/pull/95/commits/7acbb031654404c9fd711eee9974c88475de98fd
However, I still have concerns about the patch. It still lets the Cygwin
runtime operate under the assumption that it can control the console, even
though other process can interact with the same console in _overlapping_
lifecycles.
Keep in mind that the bug I reported twice involves a Cygwin process that
spawns a non-Cygwin process which writes to the console _after_ the Cygwin
process exited (and restored the console mode).
It looks plausible to me that this storing, setting, then re-setting of
the console mode by the Cygwin runtime will never cease to be fragile and
will always be prone to surface bugs that are similar, bug not identical
to the two bugs referenced above.
Therefore I fail to convince myself that the current design is okay, and
expect that, in the long run, the Cygwin runtime will stop to toggle the
console modes as if the console were not serving other, non-Cygwin
processes, too.
I do see that you changed some code so that it is only run when Cygwin is
the console owner. That triggers the following questions, answers to which
I could not find in the commit message (and neither in the diff):
- Now that the mode is left alone when the console is owned by a
non-Cygwin process, which code is broken? There must have been a reason
to change the console mode in the first place, after all.
- If it is okay to leave the console mode alone when Cygwin is _not_ the
console owner, why bother changing the mode at all?
Ciao,
Johannes
> ---
> winsup/cygwin/fhandler/console.cc | 33 ++++++++++++++++++++-----------
> 1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/console.cc
> b/winsup/cygwin/fhandler/console.cc
> index 2e19e0dd7..16352d04d 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -771,6 +771,8 @@ fhandler_console::setup ()
> con.disable_master_thread = true;
> con.master_thread_suspended = false;
> con.num_processed = 0;
> + con.curr_input_mode = tty::restore;
> + con.curr_output_mode = tty::restore;
> }
> }
>
> @@ -849,11 +851,6 @@ fhandler_console::set_input_mode (tty::cons_mode m,
> const termios *t,
> flags |= ENABLE_PROCESSED_INPUT;
> break;
> }
> - if (con.curr_input_mode != tty::cygwin && m == tty::cygwin)
> - {
> - prev_input_mode_backup = con.prev_input_mode;
> - con.prev_input_mode = oflags;
> - }
> con.curr_input_mode = m;
> SetConsoleMode (p->input_handle, flags);
> if (!(oflags & ENABLE_VIRTUAL_TERMINAL_INPUT)
> @@ -893,11 +890,6 @@ fhandler_console::set_output_mode (tty::cons_mode m,
> const termios *t,
> flags |= DISABLE_NEWLINE_AUTO_RETURN;
> break;
> }
> - if (con.curr_output_mode != tty::cygwin && m == tty::cygwin)
> - {
> - prev_output_mode_backup = con.prev_output_mode;
> - GetConsoleMode (p->output_handle, &con.prev_output_mode);
> - }
> con.curr_output_mode = m;
> acquire_attach_mutex (mutex_timeout);
> DWORD resume_pid = attach_console (con.owner);
> @@ -1845,6 +1837,12 @@ fhandler_console::open (int flags, mode_t)
> handle_set.output_handle = h;
> release_output_mutex ();
>
> + if (con.owner == GetCurrentProcessId ())
> + {
> + GetConsoleMode (get_handle (), &con.prev_input_mode);
> + GetConsoleMode (get_output_handle (), &con.prev_output_mode);
> + }
> +
> wpbuf.init ();
>
> handle_set.input_mutex = input_mutex;
> @@ -1890,6 +1888,19 @@ fhandler_console::open (int flags, mode_t)
> setenv ("TERM", "cygwin", 1);
> }
>
> + if (con.curr_input_mode != tty::cygwin)
> + {
> + prev_input_mode_backup = con.prev_input_mode;
> + GetConsoleMode (get_handle (), &con.prev_input_mode);
> + set_input_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> + }
> + if (con.curr_output_mode != tty::cygwin)
> + {
> + prev_output_mode_backup = con.prev_output_mode;
> + GetConsoleMode (get_output_handle (), &con.prev_output_mode);
> + set_output_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> + }
> +
> debug_printf ("opened conin$ %p, conout$ %p", get_handle (),
> get_output_handle ());
>
> @@ -4738,7 +4749,7 @@ fhandler_console::cons_mode_on_close (handle_set_t *p)
> NTSTATUS status =
> NtQueryInformationProcess (GetCurrentProcess (), ProcessBasicInformation,
> &pbi, sizeof (pbi), NULL);
> - if (NT_SUCCESS (status)
> + if (NT_SUCCESS (status) && cygwin_pid (con.owner)
> && !process_alive ((DWORD) pbi.InheritedFromUniqueProcessId))
> /* Execed from normal cygwin process and the parent has been exited. */
> return tty::cygwin;
> --
> 2.45.1
>
>
