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
> - 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?
> }
> - 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?
Ciao,
Johannes
> }
> state = 0;
> continue;
> }
> - else if (outbuf[i] == '\a')
> + else if (state == 4)
> + continue;
> + else
> {
> state = 0;
> continue;
> --
> 2.51.0
>
>
