Re: [PATCH] Cygwin: console: Correct previous NOFLSH fix

Hi Takashi,

Thank you for v2. It cleanly addresses the blocking `ReadConsoleInputW()`
in `sigflush()` and the underflow in `discard_key_events()`, and moving
the attach into the helper is the right cleanup. There is one concern I
would like to talk through, though: the new unconditional `tcflush
(TCIFLUSH)` in `process_sigs()`.

On Tue, 30 Jun 2026, Takashi Yano wrote:

> The previous fix for NOFLSH mode does not work as intended.
>=20
> discard_key_events(), added in "Cygwin: console: Fix NOFLSH behaviour a
> bit", loops on ReadConsoleInputW() until it has consumed the requested
> number of records, but ReadConsoleInputW() blocks while the console
> input buffer is empty. sigflush() calls it with a hard-coded count of
> one and no guarantee that a record is actually queued: in the
> master-thread path the signalling record has already been read out of
> the buffer before sigflush() runs, so the call blocks until, and then
> swallows, the user's next keystroke.
>=20
> To avoid this, this patch does not discard input when process_sigs()
> is called from cons_master_thread, where the value of `fh` is NULL,
> because discarding will be done in cons_master_thread.
>=20
> And because the ReadConsoleInputW() return value is unchecked, a failed
> read leaves the count indeterminate, so "n -=3D n1" can underflow and sp=
in.
> Check return value of ReadConsoleInputW() and abort if it fails.
>=20
> Moreover, discard_key_event(1) does not work as intended if the first
> key event is not a bKeyDown event correspoding to the signalling key.
> Use tcflush() instead(). Since the ey-strokes prior to the signalling
> key are already in the readahead buffer, so tcflush() discards only
> the signalling key.

Let's keep this sentence in mind, and continue the discussion below:

> The important point here is to discard input before
> releasing input_mutex by release_input_mutex_if_necessary(), because,
> if not, cons_master_thread starts to process key events before discardin=
g
> signalling key event because the thread can acquire input_mutex. This
> causes the signalling key is processed twice.
>=20
> One separate point: the `process_input_message()` caller wraps
> `discard_key_events()` in `acquire_attach_mutex()` + `attach_console
> (con.owner)`, but the `sigflush()` call site does not, so the
> `ReadConsoleInputW()` there runs against whatever console the calling
> process happens to be attached to. With the guard above the worst case
> is a no-op when the calling process happens not to be attached, so
> it would be more correct to move the attach into the helper itself.
>=20
> Fixes: 66324edf64a9 ("Cygwin: console: Fix NOFLSH behaviour a bit")
> Co-authored-by: Johannes Schindelin <[email protected]>
> Signed-off-by: Takashi Yano <[email protected]>
> Reviewed-by:
> ---
>  winsup/cygwin/fhandler/console.cc | 17 ++++++++---------
>  winsup/cygwin/fhandler/termios.cc | 21 +++++++++++----------
>  2 files changed, 19 insertions(+), 19 deletions(-)
>=20
> diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/=
console.cc
> index 730bb0b45..925db828c 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -1749,16 +1749,10 @@ out:
>    DWORD discard_len =3D min (total_read, i + 1);
>    /* If input is signalled, do not discard input here because
>       tcflush() is already called from line_edit(). */
> -  if (stat =3D=3D input_signalled && !(ti->c_lflag & NOFLSH))
> +  if (stat =3D=3D input_signalled)
>      discard_len =3D 0;
>    if (discard_len && (len || stat !=3D input_ok))
> -    {
> -      acquire_attach_mutex (mutex_timeout);
> -      DWORD resume_pid =3D attach_console (con.owner);
> -      discard_key_events (discard_len);
> -      detach_console (resume_pid, con.owner);
> -      release_attach_mutex ();
> -    }
> +    discard_key_events (discard_len);
>    return stat;
>  }
> =20
> @@ -1768,13 +1762,18 @@ fhandler_console::discard_key_events (size_t n)
>    DWORD discarded =3D 0;
>    INPUT_RECORD input_rec[INREC_SIZE];
>    DWORD n1 =3D min (INREC_SIZE, n);
> +  acquire_attach_mutex (mutex_timeout);
> +  DWORD resume_pid =3D attach_console (con.owner);
>    while (n)
>      {
> -      ReadConsoleInputW (get_handle (), input_rec, n1, &n1);
> +      if (!ReadConsoleInputW (get_handle (), input_rec, n1, &n1) || !n1=
)
> +	break;
>        n -=3D n1;
>        discarded +=3D n1;
>        n1 =3D min (INREC_SIZE, n);
>      }
> +  detach_console (resume_pid, con.owner);
> +  release_attach_mutex ();
>    con.num_processed -=3D min (con.num_processed, discarded);
>  }
> =20
> diff --git a/winsup/cygwin/fhandler/termios.cc b/winsup/cygwin/fhandler/=
termios.cc
> index 605258731..c59027093 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -444,10 +444,15 @@ fhandler_termios::process_sigs (char c, tty* ttyp,=
 fhandler_termios *fh)
>  	goto not_a_sig;
> =20
>        termios_printf ("got interrupt %d, sending signal %d", c, sig);
> -      if (!(ti.c_lflag & NOFLSH) && fh)
> +      if (fh)
>  	{
> -	  fh->eat_readahead (-1);
> -	  fh->discard_input ();
> +	  if (!(ti.c_lflag & NOFLSH))
> +	    {
> +	      fh->eat_readahead (-1);
> +	      fh->discard_input ();
> +	    }
> +	  if (fh->is_console ())
> +	    fh->tcflush (TCIFLUSH);

That invariant holds for records `line_edit()` has already consumed from
the console input buffer. It does not hold for records that were peeked
in the same `PeekConsoleInputW()` batch but sit at indices after the
signalling record, nor for records that arrive during the yield window
before `process_input_message()` drains the batch. Both of those cases
stay in the console input buffer and get dropped by the new `tcflush()`
along with the signalling record.

Concretely: `process_input_message()` obtains records via
`PeekConsoleInputW()` rather than consuming them, then walks
`0..total_read-1`. If a signalling character (say `^C`) sits at index
`i`, `line_edit()` has run for indices `0..i-1`, then `process_sigs()`
on the signalling byte returns `signalled`, control jumps to `out`, and
the new code sets `discard_len =3D 0`. All `total_read` records still sit
in the console input buffer at the moment `process_sigs()` runs, and
`fh->tcflush (TCIFLUSH)` (backed by `FlushConsoleInputBuffer()`) drops
all of them, regardless of `NOFLSH`.

The yield window that makes this a normal-load hazard, rather than a
corner case, is the backoff heuristic in `cons_master_thread()` with the
explicit comment "read() seems to be called. Process special keys in
`process_input_message ()`.". When it fires, `master_thread_suspended`
is set to `true` and the master thread yields; type-ahead then
accumulates in the console input buffer until `process_input_message()`
picks it up.

Compare with the master thread's own signal handling in
`cons_master_thread()` where `signalled` with `NOFLSH` set does
`goto remove_record` and writes the surviving records back, preserving
type-ahead. The new user-thread path, by contrast, flushes
unconditionally, so the two paths disagree on `NOFLSH` semantics.

For completeness, there is also a narrower reachable state where
`disable_master_thread=3Dtrue` coexists with `curr_input_mode=3Dcygwin`,
entered via the win32-input-mode DEC private mode 9001 handler which
flips `disable_master_thread` without touching `curr_input_mode`, so
the guard in `fhandler_console::bg_check()` does not fire. Narrower
than the type-ahead case, but worth flagging while we are here.

Two ways I could see to resolve it, and I have no strong preference:

(a) Gate the new `tcflush (TCIFLUSH)` on `!(ti.c_lflag & NOFLSH)`,
matching the `eat_readahead()` / `discard_input()` branch immediately
above and the reshaped `sigflush()`.

(b) Replace `tcflush (TCIFLUSH)` with a targeted single-record consume
via `ReadConsoleInputW()` (bounded and return-checked, in the shape of
the rewritten `discard_key_events()`), so only the signalling record is
dropped and `NOFLSH` type-ahead survives.

Does that reasoning make sense to you?

Ciao,
Johannes

>  	}
>        if (fh)
>  	fh->release_input_mutex_if_necessary ();
> @@ -666,13 +671,9 @@ fhandler_termios::sigflush ()
>       be NULL while this is alive.  However, we can conceivably close a
>       ctty while exiting and that will zero this. */
>    if ((!have_execed || have_execed_cygwin) && tc ()
> -      && (tc ()->getpgid () =3D=3D myself->pgid))
> -    {
> -      if (!(tc ()->ti.c_lflag & NOFLSH))
> -	tcflush (TCIFLUSH);
> -      else
> -	discard_key_events (1);
> -    }
> +      && (tc ()->getpgid () =3D=3D myself->pgid)
> +      && !(tc ()->ti.c_lflag & NOFLSH))
> +    tcflush (TCIFLUSH);
>  }
> =20
>  pid_t
> --=20
> 2.51.0
>=20
>=20