On Thu, 18 Dec 2025 09:56:34 +0100 (CET)
Johannes Schindelin wrote:
> Hi Takashi,
>
> On Thu, 18 Dec 2025, Takashi Yano wrote:
>
> > This patch fixes the bug in ESC sequence parser used when pseudo
> > console is enabled in pty_master_fwd_thread(). Previously, if
> > multiple ESC sequences exist in a fowarding chunk, the later one
> > might not be processed appropriately. In addition, the termination
> > ST (ESC \) was not supported, that is, only BEL was supported.
> >
> > Fixes: 10d083c745dd ("Cygwin: pty: Inherit typeahead data between two input
> > pipes.")
> > Reviewed-by: Corinna Vinschen <[email protected]>
> > Signed-off-by: Takashi Yano <[email protected]>
> > ---
> > winsup/cygwin/fhandler/pty.cc | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
> > index 679068ea2..3b0b4f073 100644
> > --- a/winsup/cygwin/fhandler/pty.cc
> > +++ b/winsup/cygwin/fhandler/pty.cc
> > @@ -2680,7 +2680,7 @@ fhandler_pty_master::pty_master_fwd_thread (const
> > master_fwd_thread_param_t *p)
> > int state = 0;
> > int start_at = 0;
> > for (DWORD i=0; i<rlen; i++)
>
> I wonder whether the other `for ()` loops in `pty_master_fwd_thread()`
> also need changes:
>
> -
> https://github.com/cygwin/cygwin/blob/cygwin-3.6.5/winsup/cygwin/fhandler/pty.cc#L2693-L2720
>
> -
> https://github.com/cygwin/cygwin/blob/cygwin-3.6.5/winsup/cygwin/fhandler/pty.cc#L2722-L2750
The bug was only in the parser for title set ESC sequence, I thought.
But, wait,
> -
> https://github.com/cygwin/cygwin/blob/cygwin-3.6.5/winsup/cygwin/fhandler/pty.cc#L2722-L2750
This seems to be better to fix as well for broken sequence as well as
title set sequence. Thanks for pointing this out. I'll add a separate
patch.
> > - if (outbuf[i] == '\033')
> > + if (state == 0 && outbuf[i] == '\033')
> > {
> > start_at = i;
> > state = 1;
>
> The diff context is unfortunately not wide enough to show that there is
> only this line before the next hunk:
>
> continue;
>
> > @@ -2688,12 +2688,14 @@ fhandler_pty_master::pty_master_fwd_thread (const
> > master_fwd_thread_param_t *p)
> > }
> > else if ((state == 1 && outbuf[i] == ']') ||
> > (state == 2 && outbuf[i] == '0') ||
> > - (state == 3 && outbuf[i] == ';'))
> > + (state == 3 && outbuf[i] == ';') ||
> > + (state == 4 && outbuf[i] == '\033'))
> > {
> > state ++;
> > continue;
>
> So if we encounter an ESC when the state is 1, 2 or 3, we no longer reset
> `state = 0`... That does not sound correct.
>
> And if we encounter an ESC _just_ after `ESC ] 0 ;`, we set `state = 5`
> which is then not handled other than by resetting `state = 0` at the next
> character?
Yes. If the next char is not '\\'. I think the pached code behaves so.
> > }
> > - else if (state == 4 && outbuf[i] == '\a')
> > + else if ((state == 4 && outbuf[i] == '\a')
> > + || (state == 5 && outbuf[i] == '\\'))
> > {
> > const char *helper_str = "\\bin\\cygwin-console-helper.exe";
> > if (memmem (&outbuf[start_at], i + 1 - start_at,
> > @@ -2701,11 +2703,14 @@ fhandler_pty_master::pty_master_fwd_thread (const
> > master_fwd_thread_param_t *p)
> > {
> > memmove (&outbuf[start_at], &outbuf[i+1], rlen-i-1);
> > rlen = wlen = start_at + rlen - i - 1;
> > + i = start_at - 1;
>
> I have a suspicion that _this_ is the actual bug fix necessary. Could this
> be true? If you remove the remainded of this patch and only reset `i`
> appropriately, does it fix the problem?
You are right. The most important is above one line.
Maybe other fixes should be separate patches.
Please review again for v3 patch.
> }
> state = 0;
> continue;
> }
> - else if (outbuf[i] == '\a')
> + else if (state == 4)
> + continue;
> + else
> {
> state = 0;
> continue;
--
Takashi Yano <[email protected]>
