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

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.

> In any case, I'm happy with this patch! Feel free to add my "Reviewed-by".

Pushed.

-- 
Takashi Yano <[email protected]>