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

Hi Takashi,

Thank you for v2. It correctly addresses the NOFLSH concern from the v1
review by adopting approach (b): replacing the unconditional `tcflush()`
with a targeted discard of only the records the user thread has already
handed to `line_edit`.

On Mon, 6 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
> 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]>
> ---
>=20
> v2: Use discard_key_events(0) instead of tcflush(TCIFLUSH), which
>     discards events to the current position processed.
>=20
>  winsup/cygwin/fhandler/console.cc       | 25 +++++++++++++++----------
>  winsup/cygwin/fhandler/termios.cc       | 20 ++++++++++----------
>  winsup/cygwin/local_includes/fhandler.h |  1 +
>  3 files changed, 26 insertions(+), 20 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;
>        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..6395a99ea 100644
> --- a/winsup/cygwin/fhandler/termios.cc
> +++ b/winsup/cygwin/fhandler/termios.cc
> @@ -444,10 +444,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 */);

Originally, I worried that in the non-NOFLSH case the leading `def` byte
would now survive in the console input buffer, because `discard_key_events
(0)` only consumes records 0..i. On re-reading `process_sigs`, that worry
is unfounded: after the targeted discard, the `signalled` branch proceeds
to `kill_pgrp`, which invokes `sigflush()` on the ctty, and `sigflush()`
calls `tcflush (TCIFLUSH)`, which in turn calls
`FlushConsoleInputBuffer()`. So the remaining records get dropped by the
existing `sigflush` path, and the net effect matches the pre-patch
behavior for non-NOFLSH. Please disregard that part of my earlier reply.

That said, while auditing the same area I noticed a related internal
inconsistency that is worth raising, though it is more of a "clean up
while you are here" than a v2 blocker.

The new `discard_key_events (0)` in v2 sits on the main `signalled` branch
of `process_sigs`. There are two other returns from `process_sigs` that
`line_edit`'s switch treats the same way as `signalled`:

        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 false;
            continue;

All three set `sawsig`, which eventually makes `line_edit` return
`line_edit_signalled` and drives `process_input_message` into `if (stat =
=3D=3D
input_signalled) discard_len =3D 0;`. That branch assumes the records at
0..i have already been consumed. For the main `signalled` branch that is
now true, thanks to the new `discard_key_events (0)`. But the other two
returns in `process_sigs` do neither the targeted discard nor `kill_pgrp`,
so nothing consumes those records:

        if ((with_debugger || with_debugger_nat) && need_discard_input)
          {
            if (!(ti.c_lflag & NOFLSH) && fh)
              {
                fh->eat_readahead (-1);
                fh->discard_input ();
              }
            ti.c_lflag &=3D ~FLUSHO;
            return done_with_debugger;
          }

and, at `not_a_sig:`,

        if ((ti.c_lflag & ISIG) && need_discard_input)
          {
            if (!(ti.c_lflag & NOFLSH) && fh)
              {
                fh->eat_readahead (-1);
                fh->discard_input ();
              }
            ti.c_lflag &=3D ~FLUSHO;
            return not_signalled_but_done;
          }

Neither path drains the console input buffer at all. Combined with the
unconditional `discard_len =3D 0;` in `process_input_message`, records 0..=
i
survive into the next `read()`, which re-peeks them and re-enters
`process_sigs` on the same records, potentially re-sending `CTRL_C_EVENT`.

For the record, this is not strictly a v2 regression. The pre-a42e4625e1
shape of that condition was

        if (stat =3D=3D input_signalled && !(ti->c_lflag & NOFLSH))
          discard_len =3D 0;

so for NOFLSH plus one of the debugger/ISIG paths, `discard_len` stayed
non-zero and the fallback discard at the `out:` label consumed the
records. The `!NOFLSH` sub-case was already broken pre-v1 (it relied on
`sigflush()` firing, which it does not on these two returns). v1 dropped
the `!NOFLSH` gate; v2 preserves that shape. So the NOFLSH sub-case was
regressed by v1, the non-NOFLSH sub-case is pre-existing, and neither is
fixed by v2 on its own.

The two shapes I can see for addressing this are (a) also call
`fh->discard_key_events (0)` from the `done_with_debugger` and
`not_signalled_but_done` returns in `process_sigs`, so the postcondition
"when `process_sigs` returns something that `line_edit` treats as
signalled, records 0..i have been consumed" holds uniformly across the
three return values; or (b) narrow the `if (stat =3D=3D input_signalled)
discard_len =3D 0;` condition in `process_input_message` so it only fires =
on
the code path where the discard has actually happened.

I lean slightly toward (a), because it makes the postcondition symmetric
across the three returns that `line_edit`'s switch coalesces, but I do not
feel strongly.

Do you think this is worth folding into a v3, or would you rather ship v2
as is and address it as a separate follow-up? Given how narrow the
scenario is (GDB attached to a Cygwin process reading `/dev/cons0` with
`need_discard_input` set, or a foreground pgroup containing only
non-Cygwin processes with Ctrl-C into a `foreground_special_process`), I
have no objection to splitting. Or have I missed something and this does
not need fixing at all?

Ciao,
Johannes

>  	}
>        if (fh)
>  	fh->release_input_mutex_if_necessary ();
> @@ -666,13 +670,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