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

On Sun, 5 Jul 2026 10:05:31 +0200 (CEST)
Johannes Schindelin wrote:
> 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.
> > 
> > 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.
> > 
> > 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.
> > 
> > And because the ReadConsoleInputW() return value is unchecked, a failed
> > read leaves the count indeterminate, so "n -= n1" can underflow and spin.
> > Check return value of ReadConsoleInputW() and abort if it fails.
> > 
> > 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 discarding
> > signalling key event because the thread can acquire input_mutex. This
> > causes the signalling key is processed twice.
> > 
> > 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.
> > 
> > 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(-)
> > 
> > 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 = min (total_read, i + 1);
> >    /* If input is signalled, do not discard input here because
> >       tcflush() is already called from line_edit(). */
> > -  if (stat == input_signalled && !(ti->c_lflag & NOFLSH))
> > +  if (stat == input_signalled)
> >      discard_len = 0;
> >    if (discard_len && (len || stat != input_ok))
> > -    {
> > -      acquire_attach_mutex (mutex_timeout);
> > -      DWORD resume_pid = 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;
> >  }
> >  
> > @@ -1768,13 +1762,18 @@ fhandler_console::discard_key_events (size_t n)
> >    DWORD discarded = 0;
> >    INPUT_RECORD input_rec[INREC_SIZE];
> >    DWORD n1 = min (INREC_SIZE, n);
> > +  acquire_attach_mutex (mutex_timeout);
> > +  DWORD resume_pid = attach_console (con.owner);
> >    while (n)
> >      {
> > -      ReadConsoleInputW (get_handle (), input_rec, n1, &n1);
> > +      if (!ReadConsoleInputW (get_handle (), input_rec, n1, &n1) || !n1)
> > +	break;
> >        n -= n1;
> >        discarded += n1;
> >        n1 = min (INREC_SIZE, n);
> >      }
> > +  detach_console (resume_pid, con.owner);
> > +  release_attach_mutex ();
> >    con.num_processed -= min (con.num_processed, discarded);
> >  }
> >  
> > 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;
> >  
> >        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 = 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=true` coexists with `curr_input_mode=cygwin`,
> 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.

Thanks for pointing this out.

> 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?

Yes, thanks.
I'd like to apply approach (b). Add n == 0 case in discard_key_events(),
which means discarding to the current position processed, and call it in
the process_sigs().

Please have a look v2 patch.

-- 
Takashi Yano <[email protected]>