[PATCH v2] Cygwin: pty: Fix handling of data after CSI6n response

[Takashi Yano]

Hi Johannes,

On Mon, 2 Mar 2026 14:24:39 +0100 (CET)
Johannes Schindelin wrote:
> Hi Takashi,
> 
> On Sat, 28 Feb 2026, Takashi Yano wrote:
> 
> > Previously, CSI6n was not handled correctly if the some sequences
> > are appended after the response for CSI6n. Especially, if the
> > appended sequence is a ESC sequence, which is longer than the
> > expected maximum length of the CSI6n response, the sequence will
> > not be written atomically.
> > 
> > Moreover, when the terminal's CSI 6n response and subsequent data
> > (e.g. keystrokes) arrive in the same write buffer, master::write()
> > processes all of it inside the pcon_start loop and returns early.
> > Bytes after the 'R' terminator go through per-byte line_edit() in
> > that loop instead of falling through to the `nat` pipe fast path
> > or the normal bulk `line_edit()` call. Due to this behaviour,
> > the chance of code conversion to the terminal code page for the
> > subsequent data in `to_be_read_from_nat_pipe()` case, will be lost.
> > 
> > Fix this by breaking out of the loop when 'R' is found and letting
> > the remaining data fall through to the normal write paths, which
> > are now reachable because `pcon_start` has been cleared.
> > 
> > Fixes: f20641789427 ("Cygwin: pty: Reduce unecessary input transfer.")
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > Co-authored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> Heh, I would have been fine with a mere Reviewed-by ;-)
> 
> > Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > ---
> >  winsup/cygwin/fhandler/pty.cc | 47 +++++++++++++++++++----------------
> >  1 file changed, 25 insertions(+), 22 deletions(-)
> > 
> > diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
> > index 838be4a2b..34a87c6dc 100644
> > --- a/winsup/cygwin/fhandler/pty.cc
> > +++ b/winsup/cygwin/fhandler/pty.cc
> > @@ -2137,6 +2137,8 @@ fhandler_pty_master::close (int flag)
> >  ssize_t
> >  fhandler_pty_master::write (const void *ptr, size_t len)
> >  {
> > +  size_t towrite = len;
> > +
> >    ssize_t ret;
> >    char *p = (char *) ptr;
> >    termios &ti = tc ()->ti;
> > @@ -2160,6 +2162,7 @@ fhandler_pty_master::write (const void *ptr, size_t len)
> >  
> >        DWORD n;
> >        WaitForSingleObject (input_mutex, mutex_timeout);
> > +      towrite = 0;
> 
> I originally suggested to initialize `towrite` to 0 instead of `len`
> above.
> 
> The reason why it needs to be initialized to `len`, and only be zeroed
> inside the `if (get_ttyp ()->pcon_start)` block is that you are using
> `towrite` instead of `len` in the "fall-through" block _after_ the `if
> (get_ttyp ()->pcon_start)` block, which you now fall through if the `R`
> was encountered.
> 
> And you use `towrite` instead of `len` there in that fall-through block in
> the UTF-8/code page conversion, but you still use the original `len` as
> return value from that method as well as for the `line_edit()` fall-back.
> 
> I found this a bit hard to follow.
> 
> Wouldn't it be easier to introduce a new variable `size_t orig_ret = len`
> instead of `towrite`, return `orig_ret` instead of len (and using it in
> the `line_edit()` call, too), and then adjust `len` instead of assigning
> `towrite`?

Using len in line_edit() fall-through was a bug. Thanks for pointing this
out. I'll submit a fixed version as v3 patch.

> >        for (size_t i = 0; i < len; i++)
> >  	{
> >  	  if (p[i] == '\033')
> > @@ -2171,32 +2174,33 @@ fhandler_pty_master::write (const void *ptr, size_t len)
> >  	    }
> >  	  if (state == 1)
> >  	    {
> > -	      if (ixput < wpbuf_len)
> > -		wpbuf[ixput++] = p[i];
> > -	      else
> > +	      if (ixput == wpbuf_len)
> >  		{
> >  		  if (!get_ttyp ()->req_xfer_input)
> >  		    WriteFile (to_slave_nat, wpbuf, ixput, &n, NULL);
> >  		  ixput = 0;
> > -		  wpbuf[ixput++] = p[i];
> >  		}
> > +	      wpbuf[ixput++] = p[i];
> 
> Okay, this is correct, a simple refactoring. But it does distract from the
> purpose of the patch a bit (and makes reviewing slightly more confusing
> than necessary), as it is unrelated.

Yeah, indeed. I'll remove this refactoring from this patch.

> >  	    }
> >  	  else
> >  	    line_edit (p + i, 1, ti, &ret);
> >  	  if (state == 1 && p[i] == 'R')
> >  	    state = 2;
> > -	}
> > -      if (state == 2)
> > -	{
> > -	  /* req_xfer_input is true if "ESC[6n" was sent just for
> > -	     triggering transfer_input() in master. In this case,
> > -	     the responce sequence should not be written. */
> > -	  if (!get_ttyp ()->req_xfer_input)
> > -	    WriteFile (to_slave_nat, wpbuf, ixput, &n, NULL);
> > -	  ixput = 0;
> > -	  state = 0;
> > -	  get_ttyp ()->req_xfer_input = false;
> > -	  get_ttyp ()->pcon_start = false;
> > +	  if (state == 2)
> > +	    {
> > +	      /* req_xfer_input is true if "ESC[6n" was sent just for
> > +		 triggering transfer_input() in master. In this case,
> > +		 the response sequence should not be written. */
> > +	      if (!get_ttyp ()->req_xfer_input)
> > +		WriteFile (to_slave_nat, wpbuf, ixput, &n, NULL);
> > +	      towrite = len - i - 1;
> > +	      ptr = p + i + 1;
> > +	      ixput = 0;
> > +	      state = 0;
> > +	      get_ttyp ()->req_xfer_input = false;
> > +	      get_ttyp ()->pcon_start = false;
> > +	      break;
> > +	    }
> 
> Okay, that makes sense to me, in case we reach state 2, we want to change
> `towrite` and no longer `return len` below, but instead move on to writing
> the remainder to the `nat` pipe. It is a bit unfortunate that this
> refactor makes the diff a bit harder to read than I like.

To me, breaking on 'state == 2' makes more sense than before...
This might just come down to personal preference, though.

As for readability of the patch, I agree with you.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>