Re: [PATCH v2 3/3] Cygwin: pty: Fixup pty state after a cygwin app exits
Johannes Schindelin <[email protected]> Sat, 27 Jun 2026 09:18:41 +0200 (CEST)
| Newsgroups | gmane.os.cygwin.patches |
|---|---|
| Message-ID | <[email protected]> |
Hi Takashi, Mark,
I had started working on those patches, been pulled away, and meant coming
back to them but failed. The work was tracked in
https://github.com/git-for-windows/msys2-runtime/pull/131, but I
admittedly did not find the time to complete the work earlier.
There are fixes in that PR (in addition to UI tests based on AutoHotKey
that helped me catch a couple of bugs) for the following three issues:
On Sat, 13 Jun 2026, Takashi Yano wrote:
> diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
> index b3a8d57cc..f4473bb69 100644
> --- a/winsup/cygwin/fhandler/pty.cc
> +++ b/winsup/cygwin/fhandler/pty.cc
> @@ -388,6 +388,52 @@ atexit_func (void)
> }
> }
>
> +void
> +fhandler_pty_slave::req_fixup_pcon_state (void)
> +{
> + while (true)
> + {
> + WaitForSingleObject (input_mutex, mutex_timeout);
> + if (!get_ttyp ()->pcon_start_pid)
> + break;
> + /* Another request is on going. */
> + ReleaseMutex (input_mutex);
> + yield ();
> + }
> +
> + DWORD n;
> + /* indicates that this "ESC[6n" is just for fixing-up corsor position */
> + get_ttyp ()->req_fixup_pcon_cur_pos = true;
> + get_ttyp ()->req_xfer_input = true; /* indicates that this "ESC[6n"
> + is just for transfer input */
> + get_ttyp ()->pcon_start = true;
> + get_ttyp ()->pcon_start_pid = myself->pid;
> + WriteFile (get_output_handle (), "\033[6n", 4, &n, NULL);
> + ReleaseMutex (input_mutex);
> + while (get_ttyp ()->pcon_start_pid)
> + /* wait for completion of fixing-up in master::write(). */
> + yield ();
Both of these loops are unbounded, and both depend on somebody else
clearing `pcon_start_pid`. If the master never replies (terminal closing,
broken pipe, or the previous requester died mid-handshake), the exiting
process spins forever in the second loop, and a stale slot wedges the next
exiting process in the first one. This commit also drops the
`pcon_start_pid = 0` reset that `close_pseudoconsole()` used to do, so the
stale-slot case is no longer self-healing across pcon teardown either.
Bounding both waits with a 3-second `GetTickCount64()` deadline, clearing
our own `pcon_start_pid` on timeout only if it is still ours, and
restoring the `close_pseudoconsole()` reset as a backstop makes the
pathological case degrade to a slightly stale cursor rather than a hung
exit.
The fix I would propose is in
https://github.com/git-for-windows/msys2-runtime/pull/131/changes/c366a1c02e66a242a3437f6b9335c2319c095c92:
-- snip --
From c366a1c02e66a242a3437f6b9335c2319c095c92 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Thu, 25 Jun 2026 13:41:42 +0200
Subject: [PATCH] Cygwin: pty: bound the cursor-sync round-trip so an exiting
process cannot hang
The cursor-position fixup added in "Cygwin: pty: Fixup pty state after
a cygwin app exits" runs from cleanup() on every foreground Cygwin-app
exit while a pseudo console is active, and it waits on two unbounded
loops for the master to answer the "ESC[6n" it just sent: one that
spins until the pcon_start_pid slot is free, and one that spins until
the master clears the slot again. pcon_start_pid is only ever cleared
once master::write() parses the terminal's reply, so if that reply
never comes, because the terminal is going away, the forwarding pipe
is broken, or a previous requester died mid-handshake, the exiting
process spins on yield() forever and never exits.
Bound both waits with a three second deadline using GetTickCount64(),
and on timeout clear our own pcon_start_pid slot, but only if it is
still ours, so a give-up does not stomp a later requester. Also restore
the pcon_start and pcon_start_pid reset that the same commit removed
from close_pseudoconsole(); it is the backstop that keeps a requester
which died without clearing its slot from wedging the next one. The
worst case is now a slightly stale cursor after a timeout rather than a
process that refuses to exit.
Fixes: b34394d456b6 ("Cygwin: pty: Fixup pty state after a cygwin app exits")
Assisted-by: Opus 4.8
Signed-off-by: Johannes Schindelin <[email protected]>
---
winsup/cygwin/fhandler/pty.cc | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
index c79fd1f975..669e18238b 100644
--- a/winsup/cygwin/fhandler/pty.cc
+++ b/winsup/cygwin/fhandler/pty.cc
@@ -226,6 +226,7 @@ atexit_func (void)
void
fhandler_pty_slave::req_fixup_pcon_state (void)
{
+ ULONGLONG deadline = GetTickCount64 () + 3000;
while (true)
{
WaitForSingleObject (input_mutex, mutex_timeout);
@@ -233,6 +234,10 @@ fhandler_pty_slave::req_fixup_pcon_state (void)
break;
/* Another request is on going. */
ReleaseMutex (input_mutex);
+ if (GetTickCount64 () > deadline)
+ /* A previous requester is stuck; give up this sync rather than
+ spin forever. */
+ return;
yield ();
}
@@ -245,9 +250,25 @@ fhandler_pty_slave::req_fixup_pcon_state (void)
get_ttyp ()->pcon_start_pid = myself->pid;
WriteFile (get_output_handle (), "\033[6n", 4, &n, NULL);
ReleaseMutex (input_mutex);
- while (get_ttyp ()->pcon_start_pid)
+ deadline = GetTickCount64 () + 3000;
+ while (get_ttyp ()->pcon_start_pid && GetTickCount64 () <= deadline)
/* wait for completion of fixing-up in master::write(). */
yield ();
+ /* If the master never answered (e.g. the terminal is going away),
+ clear our own request so a stale pcon_start_pid cannot wedge the
+ next requester. */
+ if (get_ttyp ()->pcon_start_pid == (pid_t) myself->pid)
+ {
+ WaitForSingleObject (input_mutex, mutex_timeout);
+ if (get_ttyp ()->pcon_start_pid == (pid_t) myself->pid)
+ {
+ get_ttyp ()->req_fixup_pcon_cur_pos = false;
+ get_ttyp ()->req_xfer_input = false;
+ get_ttyp ()->pcon_start = false;
+ get_ttyp ()->pcon_start_pid = 0;
+ }
+ ReleaseMutex (input_mutex);
+ }
}
void
@@ -4007,6 +4028,10 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
ttyp->pcon_activated = false;
ttyp->switch_to_nat_pipe = false;
ttyp->nat_pipe_owner_pid = 0;
+ /* Safety net: if a req_fixup_pcon_state() requester died without
+ clearing its slot, do not leave pcon_start_pid set forever. */
+ ttyp->pcon_start = false;
+ ttyp->pcon_start_pid = 0;
}
if (ttyp->pcon_handle_ready_event)
{
-- snap --
> +}
> +
> +void
> +fhandler_pty_master::fixup_pcon_cursor_position (int x, int y)
> +{
> + HANDLE pcon_owner = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
> + get_ttyp ()->nat_pipe_owner_pid);
> + HANDLE h_pcon_out = NULL;
> + DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
> + GetCurrentProcess (), &h_pcon_out,
> + 0, TRUE, DUPLICATE_SAME_ACCESS);
> + CloseHandle (pcon_owner);
> + DWORD target_pid = get_ttyp ()->nat_pipe_owner_pid;
> + DWORD resume_pid =
> + fhandler_pty_common::attach_console_temporarily (target_pid);
> + COORD cur_pos = {(SHORT) (x - 1), (SHORT) (y - 1)};
> + SetConsoleCursorPosition (h_pcon_out, cur_pos);
> + fhandler_pty_common::resume_from_temporarily_attach (resume_pid);
> + CloseHandle (h_pcon_out);
> +}
> +
> #define DEF_HOOK(name) static __typeof__ (name) *name##_Orig
> /* CreateProcess() is hooked for GDB etc. */
> DEF_HOOK (CreateProcessA);
> @@ -1162,6 +1208,19 @@ err_no_msg:
> bool
> fhandler_pty_slave::open_setup (int flags)
> {
> + if (get_ttyp ()->pcon_activated)
> + {
> + HANDLE pcon_owner = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
> + get_ttyp ()->nat_pipe_owner_pid);
> + DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
> + GetCurrentProcess (), &get_handle_nat (),
> + 0, TRUE, DUPLICATE_SAME_ACCESS);
> + DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
> + GetCurrentProcess (), &get_output_handle_nat (),
> + 0, TRUE, DUPLICATE_SAME_ACCESS);
> + CloseHandle (pcon_owner);
By the time we get here, `open()` has already installed real duplicates of
the cyg master-side pipe ends into `io_handle_nat` and
`output_handle_nat`. Overwriting them through `&get_handle_nat()` /
`&get_output_handle_nat()` without closing the previous values first leaks
two handles on every pcon-backed grandchild open. The `OpenProcess()`
return is also not NULL-checked: when the nat-pipe owner has already
exited, both `DuplicateHandle()` calls fail silently and leave the nat
slots NULL, which then breaks the slave's input routing in ways that are
hard to reason about after the fact.
The fix is to close the existing handles first, skip the replacement when
`OpenProcess()` returns NULL, and treat the two duplications as one
transaction so a partial failure does not leave the slave in a
half-installed state. I implemented that in
https://github.com/git-for-windows/msys2-runtime/pull/131/changes/6238d106537d6e130efe9084353850349e9c593d:
-- snip --
From 6238d106537d6e130efe9084353850349e9c593d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Thu, 25 Jun 2026 13:41:43 +0200
Subject: [PATCH] Cygwin: pty: do not leak nat handles when adopting the pcon's
in open_setup()
When a Cygwin process opens a pty slave whose pseudo console is already
active, open() has just installed duplicates of the cyg master-side
pipe ends into io_handle_nat and output_handle_nat. The pcon adoption
added in "Cygwin: pty: Fixup pty state after a cygwin app exits"
overwrites those two slots via &get_handle_nat() / &get_output_handle_nat()
without closing them first, so two handles leak on every pcon-backed
grandchild open. It also hands the result of OpenProcess() straight to
DuplicateHandle() without a NULL check, so if the nat-pipe owner has
already exited both duplications fail and leave the nat slots NULL,
which then breaks the slave's input routing.
Close the old slots before replacing them, skip the replacement
entirely when OpenProcess() returns NULL so we degrade to the handles
open() installed, and make the pair transactional so a partial success
cannot leave one original slot and one pcon slot.
Fixes: b34394d456b6 ("Cygwin: pty: Fixup pty state after a cygwin app exits")
Assisted-by: Opus 4.8
Signed-off-by: Johannes Schindelin <[email protected]>
---
winsup/cygwin/fhandler/pty.cc | 34 +++++++++++++++++++++++++++-------
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
index 669e18238b..acf7da9319 100644
--- a/winsup/cygwin/fhandler/pty.cc
+++ b/winsup/cygwin/fhandler/pty.cc
@@ -1072,13 +1072,33 @@ fhandler_pty_slave::open_setup (int flags)
{
HANDLE pcon_owner = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
get_ttyp ()->nat_pipe_owner_pid);
- DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
- GetCurrentProcess (), &get_handle_nat (),
- 0, TRUE, DUPLICATE_SAME_ACCESS);
- DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
- GetCurrentProcess (), &get_output_handle_nat (),
- 0, TRUE, DUPLICATE_SAME_ACCESS);
- CloseHandle (pcon_owner);
+ if (pcon_owner)
+ {
+ HANDLE new_in = NULL, new_out = NULL;
+ bool ok_in = DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
+ GetCurrentProcess (), &new_in,
+ 0, TRUE, DUPLICATE_SAME_ACCESS);
+ bool ok_out = DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
+ GetCurrentProcess (), &new_out,
+ 0, TRUE, DUPLICATE_SAME_ACCESS);
+ if (ok_in && ok_out)
+ {
+ /* Close the cyg master-side handles open() installed before
+ replacing them, so they do not leak. */
+ CloseHandle (get_handle_nat ());
+ CloseHandle (get_output_handle_nat ());
+ set_handle_nat (new_in);
+ set_output_handle_nat (new_out);
+ }
+ else
+ {
+ if (new_in)
+ CloseHandle (new_in);
+ if (new_out)
+ CloseHandle (new_out);
+ }
+ CloseHandle (pcon_owner);
+ }
}
set_flags ((flags & ~O_TEXT) | O_BINARY);
-- snap --
> + }
> +
> set_flags ((flags & ~O_TEXT) | O_BINARY);
> myself->set_ctty (this, flags);
> report_tty_counts (this, "opened", "");
> @@ -1171,6 +1230,9 @@ fhandler_pty_slave::open_setup (int flags)
> void
> fhandler_pty_slave::cleanup ()
> {
> + if (get_ttyp ()->pcon_activated && get_ttyp ()->getpgid () == myself->pgid)
> + req_fixup_pcon_state ();
> +
> /* This used to always call fhandler_pty_common::close when we were execing
> but that caused multiple closes of the handles associated with this pty.
> Since close_all_files is not called until after the cygwin process has
> @@ -2478,7 +2540,14 @@ fhandler_pty_master::write (const void *ptr, size_t len)
> /* 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)
> + if (get_ttyp ()->req_fixup_pcon_cur_pos)
> + {
> + int x, y;
> + sscanf (wpbuf, "\033[%d;%dR", &y, &x);
> + fixup_pcon_cursor_position (x, y);
The `sscanf()` return is dropped, so a malformed or truncated
cursor-position reply hands uninitialised `x` and `y` straight into
`SetConsoleCursorPosition()` via the COORD cast.
`fixup_pcon_cursor_position()` itself has the same `OpenProcess()` NULL
hazard as above, plus an unchecked `DuplicateHandle()` whose `h_pcon_out`
is then used unconditionally.
Gating the call on `sscanf (...) == 2`, clamping the coordinates into the
valid SHORT range before the COORD cast, and adding the missing NULL
checks in the helper closes all three holes:
https://github.com/git-for-windows/msys2-runtime/pull/131/changes/5de332d1b7c289ff9f3b02f2dedc9e9842fbbf04
-- snip --
From 5de332d1b7c289ff9f3b02f2dedc9e9842fbbf04 Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <[email protected]>
Date: Thu, 25 Jun 2026 13:41:44 +0200
Subject: [PATCH] Cygwin: pty: validate the cursor-position reply before moving
the pcon cursor
The CSI6n reply handler added in "Cygwin: pty: Fixup pty state after a
cygwin app exits" runs sscanf() on the terminal's response but ignores
its return value, so a malformed or partial reply leaves the x and y
locals uninitialised and hands them to SetConsoleCursorPosition(),
which is exactly the cursor corruption the commit set out to prevent.
Only call the fixup when sscanf() reports both coordinates parsed, and
in fixup_pcon_cursor_position() clamp the coordinates into the valid
SHORT range before the COORD cast so a stray reply cannot wrap into a
negative position. While there, check OpenProcess() for NULL (the
nat-pipe owner may have exited) and check the DuplicateHandle() result
instead of using a possibly-NULL screen-buffer handle.
Fixes: b34394d456b6 ("Cygwin: pty: Fixup pty state after a cygwin app exits")
Assisted-by: Opus 4.8
Signed-off-by: Johannes Schindelin <[email protected]>
---
winsup/cygwin/fhandler/pty.cc | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
index acf7da9319..522f46e0f2 100644
--- a/winsup/cygwin/fhandler/pty.cc
+++ b/winsup/cygwin/fhandler/pty.cc
@@ -274,12 +274,23 @@ fhandler_pty_slave::req_fixup_pcon_state (void)
void
fhandler_pty_master::fixup_pcon_cursor_position (int x, int y)
{
+ /* A malformed or out-of-range reply must not be turned into a wrapped
+ negative COORD. */
+ if (x < 1 || y < 1 || x > 0x7fff || y > 0x7fff)
+ return;
HANDLE pcon_owner = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
get_ttyp ()->nat_pipe_owner_pid);
+ if (!pcon_owner)
+ /* The nat-pipe owner is gone; nothing to sync to. */
+ return;
HANDLE h_pcon_out = NULL;
- DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
- GetCurrentProcess (), &h_pcon_out,
- 0, TRUE, DUPLICATE_SAME_ACCESS);
+ if (!DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_out,
+ GetCurrentProcess (), &h_pcon_out,
+ 0, TRUE, DUPLICATE_SAME_ACCESS))
+ {
+ CloseHandle (pcon_owner);
+ return;
+ }
CloseHandle (pcon_owner);
DWORD target_pid = get_ttyp ()->nat_pipe_owner_pid;
DWORD resume_pid =
@@ -2424,8 +2435,8 @@ fhandler_pty_master::write (const void *ptr, size_t len)
if (get_ttyp ()->req_fixup_pcon_cur_pos)
{
int x, y;
- sscanf (wpbuf, "\033[%d;%dR", &y, &x);
- fixup_pcon_cursor_position (x, y);
+ if (sscanf (wpbuf, "\033[%d;%dR", &y, &x) == 2)
+ fixup_pcon_cursor_position (x, y);
get_ttyp ()->req_fixup_pcon_cur_pos = false;
}
else if (!get_ttyp ()->req_xfer_input)
-- snap --
Again, I am sorry for the lack of my presence in this thread!
Ciao,
Johannes
> + get_ttyp ()->req_fixup_pcon_cur_pos = false;
> + }
> + else if (!get_ttyp ()->req_xfer_input)
> WriteFile (to_slave_nat, wpbuf, ixput, &n, NULL);
> ixput = 0;
> state = 0;
> @@ -4100,8 +4169,6 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
> ttyp->pcon_activated = false;
> ttyp->switch_to_nat_pipe = false;
> ttyp->nat_pipe_owner_pid = 0;
> - ttyp->pcon_start = false;
> - ttyp->pcon_start_pid = 0;
> }
> if (ttyp->pcon_handle_ready_event)
> {
> diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h
> index 322592bf1..2fa30cbce 100644
> --- a/winsup/cygwin/local_includes/fhandler.h
> +++ b/winsup/cygwin/local_includes/fhandler.h
> @@ -2533,6 +2533,7 @@ class fhandler_pty_slave: public fhandler_pty_common
> void setpgid_aux (pid_t pid);
> static void release_ownership_of_nat_pipe (tty *ttyp, fhandler_termios *fh);
> void replace_nat_handles (HANDLE new_input, HANDLE new_output);
> + void req_fixup_pcon_state (void);
> };
>
> #define __ptsname(buf, unit) __small_sprintf ((buf), "/dev/pty%d", (unit))
> @@ -2639,6 +2640,7 @@ public:
> void get_master_fwd_thread_param (master_fwd_thread_param_t *p);
> bool need_send_ctrl_c_event ();
> void apply_line_edit_to_transferred_input ();
> + void fixup_pcon_cursor_position (int x, int y);
> };
>
> class fhandler_dev_null: public fhandler_base
> diff --git a/winsup/cygwin/local_includes/tty.h b/winsup/cygwin/local_includes/tty.h
> index 507f7772e..c5102eb81 100644
> --- a/winsup/cygwin/local_includes/tty.h
> +++ b/winsup/cygwin/local_includes/tty.h
> @@ -145,6 +145,7 @@ private:
> xfer_dir pty_input_state;
> bool discard_input;
> bool stop_fwd_thread;
> + bool req_fixup_pcon_cur_pos;
>
> public:
> HANDLE from_master_nat () const { return _from_master_nat; }
> --
> 2.51.0
>
>