Re: [PATCH] Cygwin: console: Fix regression in console input

On Fri, 7 Aug 2026 01:25:51 +0900
Takashi Yano wrote:
> Hi Johannes,
> 
> Thanks for reviewing!
> 
> On Thu, 6 Aug 2026 16:21:49 +0200 (CEST)
> Johannes Schindelin wrote:
> > Hi Takashi,
> > 
> > On Mon, 3 Aug 2026, Takashi Yano wrote:
> > 
> > > The commit fac73911f5a0 ("Cygwin: console: Fix typeahead input for
> > > bash") introduced a bug where select() consumes some input chars in
> > > canonical mode, preventing read() from reading them. This is due to
> > > discarding input events when process_input_message() does not return
> > > `input_ok` even if it is called from select().
> > > The basic idea of that commit was making process_input_message()
> > > not to store processed chars into readahead buffer. This was not
> > > correct because the key input events ware processed twice, once by
> > > select() and again by read(). Thus even if that commit worked as
> > > intended, the side effect such as input echo would be applied twice.
> > > 
> > > With this patch, process_input_message() handles only the minimum
> > > necessary of input events in both cases, those processed by select()
> > > and those processed by read(). To achive this behaviour, the function
> > > returns without processing when `input_ready` is already satisfied,
> > > or after it has processed the specified number of chars.
> > > 
> > > Addresses: https://cygwin.com/pipermail/cygwin/2026-August/259915.html
> > > Reported-by: Steven Doerfler <[email protected]>
> > > Fixes: fac73911f5a0 ("Cygwin: console: Fix typeahead input for bash")
> > > Signed-off-by: Takashi Yano <[email protected]>
> > > Revewed-by:
> > 
> > This explanation, as well as the diff, look sound to me.
> > 
> > You may want to fix the typos "Revewed" -> "Reviewed", "ware" -> "were"
> > and "achive" -> "achieve", but those are tiny nits.
> > 
> > You could also fix the pre-existing typo in the comment "recored" ->
> > "recorded" while at it, but again, that's just a nit.
> > 
> > > ---
> > >  winsup/cygwin/fhandler/console.cc | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
> > > index 0219a37ef..3904fbd57 100644
> > > --- a/winsup/cygwin/fhandler/console.cc
> > > +++ b/winsup/cygwin/fhandler/console.cc
> > > @@ -1406,7 +1406,6 @@ fhandler_console::input_states
> > >  fhandler_console::process_input_message (size_t len)
> > >  {
> > >    char tmp[60];
> > > -  size_t num_chars = 0;
> > >  
> > >    if (!shared_console_info[unit])
> > >      return input_error;
> > > @@ -1429,6 +1428,10 @@ fhandler_console::process_input_message (size_t len)
> > >        return input_error;
> > >      }
> > >  
> > > +  /* len == 0 if called from select.cc:peek_console() */
> > > +  if (input_ready && (len == 0 || (get_ttyp ()->ti.c_lflag & ICANON)))
> > > +    return input_ok;
> > 
> > It is worth pointing out that while this `input_ready &&` part looks
> > superfluous (both callers guard the call behind `!input_ready`), it is a
> > concurrency guard: a second thread reading the same console fd can set
> > `inpuy_ready` in the window between `read()`'s lock-free `while
> > (!input_ready)` check and `acquire_input_mutex`.
> > 
> > Maybe worth an extra code comment?
> 
> I added short comment here. Thanks.
> 
> > > +
> > >    for (i = 0; i < total_read; i ++)
> > >      {
> > >        DWORD nread = 1;
> > > @@ -1794,7 +1797,6 @@ fhandler_console::process_input_message (size_t len)
> > >  	}
> > >  
> > >        num_input_events_processed = i + 1;
> > > -      num_chars += nread;
> > >        if (toadd)
> > >  	{
> > >  	  ssize_t ret;
> > > @@ -1813,21 +1815,17 @@ fhandler_console::process_input_message (size_t len)
> > >  	    }
> > >  	}
> > >        /* len == 0 if called from select.cc:peek_console() */
> > > -      if (len && num_chars >= len)
> > > +      if (input_ready && (len == 0 || con_ra.ralen >= len))
> > 
> > Something GPT 5.6 Sol stumbled over, before noticing that this is a
> > pre-existing issue: the raw multi-pass caller passes full `buflen` (not
> > `buflen - copied_chars`). That was already there before this patch (and
> > both GPT and Opus think that this is incorrect, I haven't had time to wrap
> > my head around it), and the new `ralen`-based check is actually
> > neutral-to-better; and `tcflush(TCIFLUSH)` doesn't flush `con_ra`.
> > 
> > I'm fairly certain that this is out of scope for this here bug fix, if it
> > is an issue at all (what do you think? Is this `buflen` issue real? Should
> > `tcflush()` also flush `con_ra`?).
> 
> Thanks for finding this. I rechecked here and concluded passing `buflen`
> for process_input_message() is correct. This is because, the length check
> in the process_input_message is:
> 
>       if (input_ready && (len == 0 || con_ra.ralen >= len))
>         goto out;
> 
> After the `while` loop, the chars in readahead data of length `con_ra.ralen`
> will be copied into buffer of `buflen`. So the `len` should be total buffer
> size here.
> 
> As for tcflush(), let me check. Please wait for a while.

`tcflush()` seems to need clearing readahead buffer and `input_ready` flag.
Without that, select()->tcflush()->read() does not block at read() and
returns the contents of readahead buffer.

I'll submit another patch shortly.

-- 
Takashi Yano <[email protected]>