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

Hi Takashi,

Thank you for v3. Both concerns from v2 are addressed, and the "stty intr
^x; cat | non-cygwin-app" case now behaves as expected.

I have two non-blocking observations further below, and a question: Out of
curiosity, not a blocker: how does the `with_debugger_nat` branch actually
get reached in practice? gdb normally reads the console only at its own
prompt, i.e. when the inferior is stopped and thus not foreground, so the
pre-conditions do not obviously line up.

  Reviewed-by: Johannes Schindelin <[email protected]>

On Wed, 8 Jul 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 discard_key_events(0) instead. This means discarding input events
> to the current position processed. Since the key-strokes prior to the
> signalling key are already in the readahead buffer, so this call discard=
s
> only the signalling key. 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
> discarding 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
> This patch also fixes two more special cases. One is done_with_debugger
> case. When `gdb cat` is executed and the `cat` is running, Ctrl-C
> discards all the key events including the events after Ctrl-C. This
> is because tcflush() is used for the purpose. Use discard_key_events(0)
> instead. The other case is not_signalled_but_done case. Previously,
> when `cat | non-cygwin-app` is executed and Ctrl-C is pressed, but
> the `Ctrl-C` is not VINTR, line_edit() wrongly returned
> line_edit_signalled even though `cat` is not signalled by Ctrl-C.
> In this case, `cat` should receive Ctrl-C as a input char, while
> `non-cygwin-app` has been killed by Ctrl-C. Fix this in line_edit().
>=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: Johannes Schindelin <[email protected]>
> ---
> v2: Use discard_key_events(0) instead of tcflush(TCIFLUSH), which
>     discards events to the current position processed.
> v3: Fix behaviour of two special cases: done_with_debugger and
>     not_signalled_but_done
>=20
>  winsup/cygwin/fhandler/console.cc       | 25 +++++++++++++---------
>  winsup/cygwin/fhandler/termios.cc       | 28 ++++++++++++++-----------
>  winsup/cygwin/local_includes/fhandler.h |  1 +
>  3 files changed, 32 insertions(+), 22 deletions(-)
>=20
> diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/=
console.cc
> index 730bb0b45..cc4591c14 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -1718,6 +1718,7 @@ fhandler_console::process_input_message (size_t le=
n)
>  	  continue;
>  	}
> =20
> +      num_input_events_processed =3D i + 1;

The new counter member is missing from the constructor's init list. It is
safe in practice because `cnew()` zero-fills, but adding it explicitly
would match the surrounding style.

>        num_chars +=3D nread;
>        if (toadd)
>  	{
> @@ -1748,17 +1749,11 @@ out:
>    /* Discard processed recored. */
>    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))
> +     discard_key_events() is already called from line_edit(). */
> +  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
> @@ -1766,15 +1761,25 @@ void
>  fhandler_console::discard_key_events (size_t n)
>  {
>    DWORD discarded =3D 0;
> +  if (n =3D=3D 0)
> +    {
> +      n =3D num_input_events_processed;
> +      num_input_events_processed =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..9971bb1d9 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -353,7 +353,10 @@ fhandler_termios::process_sigs (char c, tty* ttyp, =
fhandler_termios *fh)
>  	      fhandler_pty_common::attach_console_temporarily (p->dwProcessId)=
;
>  	  if (fh && p =3D=3D myself && being_debugged ())
>  	    { /* Avoid deadlock in gdb on console. */
> -	      fh->tcflush(TCIFLUSH);
> +	      if (fh->is_console ())
> +		fh->discard_key_events (0 /* to current position */);
> +	      else
> +		fh->tcflush(TCIFLUSH);
>  	      fh->release_input_mutex_if_necessary ();
>  	    }
>  	  /* CTRL_C_EVENT does not work for the process started with
> @@ -444,10 +447,14 @@ 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 ();
> +	    }
> +	  fh->discard_key_events (0 /* to current position */);
>  	}
>        if (fh)
>  	fh->release_input_mutex_if_necessary ();
> @@ -525,11 +532,12 @@ fhandler_termios::line_edit (const char *rptr, siz=
e_t nread, termios& ti,
>        switch (process_sigs (c, get_ttyp (), this))
>  	{
>  	case signalled:
> -	case not_signalled_but_done:
>  	case done_with_debugger:
>  	  sawsig =3D true;
>  	  get_ttyp ()->output_stopped &=3D ~BY_VSTOP;
>  	  continue;
> +	case not_signalled_but_done:
> +	  break;

Dropping `not_signalled_but_done` from the shared switch case also drops
the clearing of `BY_VSTOP` on this path. Probably fine, since no cygwin
signal is delivered here, but a sentence in the commit message would save
future git-blame archaeology.

Ciao,
Johannes

>  	case not_signalled_with_nat_reader:
>  	  disable_eof_key =3D true;
>  	  break;
> @@ -666,13 +674,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
> diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/loc=
al_includes/fhandler.h
> index 8e9cbef4b..d11b3ec4f 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -2201,6 +2201,7 @@ private:
>    HANDLE input_mutex, output_mutex;
>    handle_set_t handle_set;
>    _minor_t unit;
> +  size_t num_input_events_processed;
> =20
>    /* Used when we encounter a truncated multi-byte sequence.  The
>       lead bytes are stored here and revisited in the next write call. *=
/
> --=20
> 2.51.0
>=20
>=20