Re: [PATCH v4] Cygwin: console: Fix undesired mode change at exit of non-cygwin apps

Hi Takashi,

On Thu, 16 Jul 2026, Takashi Yano wrote:

> Previously, if two non-cygwin apps are started and one of them
> exits first, the other one loosed appropriate console mode, since
> the first one restored it to tty::cygwin. This patch counts the
> active console process whose pgid is pgid of the tty and if the
> result is zero (means the last non-cygwin foreground process),
> restore console mode.

Thank you for v4 and for the follow-up correction. I verified that the
published v4 applies cleanly to 0d516c2b1f4d and compiles and links a
`new-cygwin1.dll`. I have not been able to establish the runtime behavior
yet, so the following is from reading the code.

About the follow-up, to remove that `CloseHandle ()` call: agreed. That
correction addresses the duplicate CloseHandle() only, though; the two
blocking issues below are independent of it.

> To avoid race issue between apps modifying
> console mode simultaneously, this patch also introduce a mutex
> named `cons_mode_mutex`.

Here is the first blocking problem, and it is a direct consequence of
"guard all mode changes". The new mutex is not acquired in a consistent
order with respect to `output_mutex`.

>=20
> Fixes: 48285aa36c2c ("Cygwin: console: Fix handling of Ctrl-S in Win7.")
> Signed-off-by: Takashi Yano <[email protected]>
> Reviewed-by: Johannes Schindelin <[email protected]>
> ---
> v2: Stop counting up/down the counter by itself.
>     Use num_active_non_cygwin_apps() instead.
> v3: Guard setup_for_non_cygwin_app() by cons_mode_mutex as well.
> v4: Guard all mode changes in console by cons_mode_mutex.
>=20
>  winsup/cygwin/fhandler/console.cc       | 87 ++++++++++++++++++++++++-
>  winsup/cygwin/local_includes/fhandler.h |  2 +
>  2 files changed, 86 insertions(+), 3 deletions(-)
>=20
> diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/=
console.cc
> index d4c87f29f..5b9a87ebd 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -977,15 +977,59 @@ fhandler_console::setup_for_non_cygwin_app ()
>       console mode. */
>    if (get_ttyp ()->getpgid () =3D=3D myself->pgid)
>      {
> +      WaitForSingleObject (cons_mode_mutex, INFINITE);
>        set_disable_master_thread (true, this);
>        set_input_mode (tty::native, &tc ()->ti, get_handle_set ());
>        set_output_mode (tty::native, &tc ()->ti, get_handle_set ());
> +      ReleaseMutex (cons_mode_mutex);

In this path, `cons_mode_mutex` is taken first, and then
`set_output_mode()` acquires `output_mutex` underneath it. The same
nesting holds for `cleanup_for_non_cygwin_app()`, `bg_check()`, `open()`,
`post_open_setup()`, `tcsetattr()`, and `set_console_mode_to_native()`:
`cons_mode_mutex` first, `output_mutex` second.

>      }
>  }
> =20
> +static int
> +num_active_non_cygwin_apps (pid_t pgid)
> +{
> +  tmp_pathbuf tp;
> +  DWORD *list =3D (DWORD *) tp.c_get ();
> +  const DWORD buf_size =3D NT_MAX_PATH / sizeof (DWORD);
> +
> +  DWORD buf_size1 =3D 1;
> +  DWORD num;
> +  /* The buffer of too large size does not seem to be expected by new c=
ondrv.
> +     https://github.com/microsoft/terminal/issues/18264#issuecomment-25=
15448548
> +     Use the minimum buffer size in the loop. */
> +  while ((num =3D GetConsoleProcessList (list, buf_size1)) > buf_size1)
> +    {
> +      if (num > buf_size)
> +	return 0;
> +      buf_size1 =3D num;
> +    }
> +  if (num =3D=3D 0)
> +    return 0;
> +
> +  int cnt =3D 0;
> +  for (DWORD i =3D 0; i < num; i++)
> +    {
> +      pinfo p (cygwin_pid (list[i]));
> +      if (!!p && p->pgid =3D=3D pgid && ISSTATE (p, PID_NOTCYGWIN))
> +	cnt++;
> +    }
> +  return cnt;
> +}
> +
>  void
>  fhandler_console::cleanup_for_non_cygwin_app (handle_set_t *p)
>  {
> +  if (cygheap->ctty->tc()->pgid !=3D myself->pgid)
> +    return;
> +
> +  WaitForSingleObject (p->cons_mode_mutex, INFINITE);
> +  if (num_active_non_cygwin_apps (cygheap->ctty->tc()->pgid))
> +    {
> +      ReleaseMutex (p->cons_mode_mutex);

The second blocking problem is a race window around process creation.
`setup_for_non_cygwin_app()` runs before `CreateProcessW()` and releases
`cons_mode_mutex` before the spawned process becomes visible: it is not
yet in `GetConsoleProcessList()`, and there is no `pinfo` with
`PID_NOTCYGWIN` and the matching `pgid` for it. During that interval a
concurrent `cleanup_for_non_cygwin_app()` can count zero non-Cygwin apps
and restore the Cygwin console mode, which is exactly the regression this
patch sets out to fix. Keep in mind that the count is only reliable once
the spawned process is both created and published. The pending native-mode
state needs to remain represented across process creation and publication,
with a rollback if `CreateProcessW()` fails.

Two non-blocking points on this `num_active_non_cygwin_apps()` helper.

First, latency. On each call this normally does two
`GetConsoleProcessList()` calls (more if the process count changes between
them) plus O(N) `cygwin_pid()` and `pinfo` construction, all under
`cons_mode_mutex`, and v4 broadens who has to wait for that mutex. I am
not claiming an observed slowdown; I have not measured it. Did you? If
not, a cheap fast path seems worthwhile: this loop counts every match but
only the question "is there at least one?" matters, so returning on the
first match, and skipping `pinfo` construction entirely when
`cygwin_pid()` returns 0, would bound the common case.

Second, error semantics. This returns 0 for three distinct situations: a
genuine "no matching process", an API failure (`num =3D=3D 0`), and output
exceeding `buf_size`. But 0 is precisely the value that authorizes the
caller to restore the Cygwin mode. Conflating "there are no non-Cygwin
apps" with "I could not find out" means an enumeration failure silently
triggers a mode restore. Please give the helper a distinct
"unavailable/error" result so the caller can decline to restore when the
count is not trustworthy.

Ciao,
Johannes

> +      CloseHandle (p->cons_mode_mutex);
> +      return;
> +    }
> +
>    const _minor_t unit =3D p->unit;
>    termios dummy =3D {0, };
>    termios *ti =3D shared_console_info[unit] ?
> @@ -999,6 +1043,7 @@ fhandler_console::cleanup_for_non_cygwin_app (handl=
e_set_t *p)
>      set_output_mode (conmode, ti, p);
>    if (con.curr_input_mode !=3D conmode)
>      set_input_mode (conmode, ti, p);
> +  ReleaseMutex (p->cons_mode_mutex);
>  }
> =20
>  /* Return the tty structure associated with a given tty number.  If the
> @@ -1055,6 +1100,10 @@ fhandler_console::setup_io_mutex (void)
>    if (res =3D=3D WAIT_OBJECT_0)
>      release_output_mutex ();
> =20
> +  shared_name (buf, "cygcons.cons_mode.mutex", get_minor ());
> +  if (!cons_mode_mutex)
> +    cons_mode_mutex =3D CreateMutex (&sec_none, FALSE, buf);
> +
>    extern HANDLE attach_mutex;
>    if (!attach_mutex)
>      attach_mutex =3D CreateMutex (&sec_none_nih, FALSE, NULL);
> @@ -1189,6 +1238,7 @@ fhandler_console::bg_check (int sig, bool dontsign=
al)
>    /* Setting-up console mode for cygwin app. This is necessary if the
>       cygwin app and other non-cygwin apps are started simultaneously
>       in the same process group. */
> +  WaitForSingleObject (cons_mode_mutex, INFINITE);
>    if (sig =3D=3D SIGTTIN && con.curr_input_mode !=3D tty::cygwin)
>      {
>        set_disable_master_thread (false, this);
> @@ -1196,6 +1246,7 @@ fhandler_console::bg_check (int sig, bool dontsign=
al)
>      }
>    if (sig =3D=3D SIGTTOU && con.curr_output_mode !=3D tty::cygwin)
>      set_output_mode (tty::cygwin, &tc ()->ti, get_handle_set ());
> +  ReleaseMutex (cons_mode_mutex);
> =20
>    return fhandler_termios::bg_check (sig, dontsignal);
>  }
> @@ -2010,6 +2061,7 @@ fhandler_console::open (int flags, mode_t)
>    if (in_is_console)
>      CloseHandle (h_in);
> =20
> +  WaitForSingleObject (cons_mode_mutex, INFINITE);
>    if (in_is_console && con.curr_input_mode !=3D tty::cygwin)
>      {
>        prev_input_mode_backup =3D con.prev_input_mode;
> @@ -2022,6 +2074,7 @@ fhandler_console::open (int flags, mode_t)
>        GetConsoleMode (get_output_handle (), &con.prev_output_mode);
>        set_output_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
>      }
> +  ReleaseMutex (cons_mode_mutex);
> =20
>    debug_printf ("opened conin$ %p, conout$ %p", get_handle (),
>  		get_output_handle ());
> @@ -2105,6 +2158,7 @@ fhandler_console::open_setup (int flags)
>        handle_set.output_handle =3D get_output_handle ();
>        handle_set.input_mutex =3D input_mutex;
>        handle_set.output_mutex =3D output_mutex;
> +      handle_set.cons_mode_mutex =3D cons_mode_mutex;
>        handle_set.unit =3D unit;
>      }
>    return fhandler_base::open_setup (flags);
> @@ -2114,6 +2168,7 @@ void
>  fhandler_console::post_open_setup (int fd)
>  {
>    /* Setting-up console mode for cygwin app started from non-cygwin app=
. */
> +  WaitForSingleObject (cons_mode_mutex, INFINITE);
>    if (fd =3D=3D 0)
>      {
>        set_disable_master_thread (false, this);
> @@ -2121,6 +2176,7 @@ fhandler_console::post_open_setup (int fd)
>      }
>    else if (fd =3D=3D 1 || fd =3D=3D 2)
>      set_output_mode (tty::cygwin, &get_ttyp ()->ti, &handle_set);
> +  ReleaseMutex (cons_mode_mutex);
> =20
>    fhandler_base::post_open_setup (fd);
>  }
> @@ -2135,11 +2191,13 @@ fhandler_console::close (int flag)
>    if (shared_console_info[unit] && (dev_t) myself->ctty =3D=3D get_devi=
ce ()
>        && cons_mode_on_close (&handle_set) =3D=3D tty::restore)
>      {
> +      WaitForSingleObject (cons_mode_mutex, INFINITE);
>        set_disable_master_thread (true, this);
>        if (con.curr_output_mode !=3D tty::restore)
>  	set_output_mode (tty::restore, &get_ttyp ()->ti, &handle_set);

But `close()` here, and `char_command()` below, are already holding
`output_mutex` at the point they reach this code, and only then acquire
`cons_mode_mutex`. That is the opposite order.

>        if (con.curr_input_mode !=3D tty::restore)
>  	set_input_mode (tty::restore, &get_ttyp ()->ti, &handle_set);
> +      ReleaseMutex (cons_mode_mutex);
>      }
> =20
>    if (shared_console_info[unit] && con.owner =3D=3D GetCurrentProcessId=
 ())
> @@ -2196,6 +2254,8 @@ fhandler_console::close (int flag)
>    input_mutex =3D NULL;
>    CloseHandle (output_mutex);
>    output_mutex =3D NULL;
> +  CloseHandle (cons_mode_mutex);
> +  cons_mode_mutex =3D NULL;
> =20
>    pcon_hand_over_proc ();
> =20
> @@ -2369,10 +2429,12 @@ int
>  fhandler_console::tcsetattr (int a, struct termios const *t)
>  {
>    get_ttyp ()->ti =3D *t;
> +  WaitForSingleObject (cons_mode_mutex, INFINITE);
>    if (con.curr_input_mode =3D=3D tty::cygwin)
>      set_input_mode (tty::cygwin, t, &handle_set);
>    if (con.curr_output_mode =3D=3D tty::cygwin)

Two processes can therefore wait on each other indefinitely: one holding
`output_mutex` and waiting for `cons_mode_mutex`, the other holding
`cons_mode_mutex` and waiting for `output_mutex`. We need one consistent
lock order across all of these sites. I do not want to prescribe a
specific redesign as unquestionably correct; whether that means taking
`output_mutex` before `cons_mode_mutex` everywhere, or narrowing
`cons_mode_mutex` so it never nests over `output_mutex`, is your call. But
the current mixed order is a showstopper.

>      set_output_mode (tty::cygwin, t, &handle_set);
> +  ReleaseMutex (cons_mode_mutex);
>    return 0;
>  }
> =20
> @@ -3140,10 +3202,12 @@ fhandler_console::char_command (char c)
>  		    con.cursor_key_app_mode =3D (c =3D=3D 'h');
>  		  if (con.args[i] =3D=3D 9001) /* win32-input-mode (https://github.co=
m/microsoft/terminal/blob/main/doc/specs/%234999%20-%20Improved%20keyboard=
%20handling%20in%20Conpty.md) */
>  		    {
> +		      WaitForSingleObject (cons_mode_mutex, INFINITE);
>  		      set_disable_master_thread (c =3D=3D 'h', this);
>  		      if (con.curr_input_mode =3D=3D tty::cygwin)
>  			set_input_mode (tty::cygwin,
>  					&tc ()->ti, get_handle_set ());
> +		      ReleaseMutex (cons_mode_mutex);
>  		    }
>  		}
>  	      /* Call fix_tab_position() if screen has been alternated. */
> @@ -4475,10 +4539,13 @@ fhandler_console::set_console_mode_to_native ()
>  	fhandler_console *cons =3D (fhandler_console *) (fhandler_base *) cfd;
>  	if (cons->get_device () =3D=3D cons->tc ()->getntty ())
>  	  {
> +	    const fhandler_console::handle_set_t *p =3D cons->get_handle_set (=
);
> +	    WaitForSingleObject (p->cons_mode_mutex, INFINITE);
>  	    set_disable_master_thread (true, cons);
>  	    termios *cons_ti =3D &cons->tc ()->ti;
> -	    set_input_mode (tty::native, cons_ti, cons->get_handle_set ());
> -	    set_output_mode (tty::native, cons_ti, cons->get_handle_set ());
> +	    set_input_mode (tty::native, cons_ti, p);
> +	    set_output_mode (tty::native, cons_ti, p);
> +	    ReleaseMutex (p->cons_mode_mutex);
>  	    break;
>  	  }
>        }
> @@ -4536,7 +4603,16 @@ static FARPROC
>  GetProcAddress_Hooked (HMODULE h, LPCSTR n)
>  {
>    if (strcmp(n, "RequestTermConnector") =3D=3D 0)
> -    fhandler_console::set_disable_master_thread (true);
> +    {
> +      char buf[MAX_PATH];
> +      const _minor_t unit =3D cygheap->ctty->get_minor ();
> +      shared_name (buf, "cygcons.cons_mode.mutex", unit);
> +      HANDLE cons_mode_mutex =3D CreateMutex (&sec_none, FALSE, buf);
> +      WaitForSingleObject (cons_mode_mutex, INFINITE);
> +      fhandler_console::set_disable_master_thread (true);
> +      ReleaseMutex (cons_mode_mutex);
> +      CloseHandle (cons_mode_mutex);
> +    }
>    return GetProcAddress_Orig (h, n);
>  }
> =20
> @@ -4817,6 +4893,9 @@ fhandler_console::get_duplicated_handle_set (handl=
e_set_t *p)
>    DuplicateHandle (GetCurrentProcess (), output_mutex,
>  		   GetCurrentProcess (), &p->output_mutex,
>  		   0, FALSE, DUPLICATE_SAME_ACCESS);
> +  DuplicateHandle (GetCurrentProcess (), cons_mode_mutex,
> +		   GetCurrentProcess (), &p->cons_mode_mutex,
> +		   0, FALSE, DUPLICATE_SAME_ACCESS);
>    p->unit =3D unit;
>  }
> =20
> @@ -4833,6 +4912,8 @@ fhandler_console::close_handle_set (handle_set_t *=
p)
>    p->input_mutex =3D NULL;
>    CloseHandle (p->output_mutex);
>    p->output_mutex =3D NULL;
> +  CloseHandle (p->cons_mode_mutex);
> +  p->cons_mode_mutex =3D NULL;
>  }
> =20
>  bool
> diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/loc=
al_includes/fhandler.h
> index d11b3ec4f..9c891863f 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -2023,6 +2023,7 @@ class fhandler_termios: public fhandler_base
>      HANDLE output_handle;
>      HANDLE input_mutex;
>      HANDLE output_mutex;
> +    HANDLE cons_mode_mutex;
>      _minor_t unit;
>    };
>    class spawn_worker
> @@ -2199,6 +2200,7 @@ private:
>    static console_state *shared_console_info[MAX_CONS_DEV + 1];
>    static bool invisible_console;
>    HANDLE input_mutex, output_mutex;
> +  HANDLE cons_mode_mutex;
>    handle_set_t handle_set;
>    _minor_t unit;
>    size_t num_input_events_processed;
> --=20
> 2.51.0
>=20
>=20