Re: [PATCH v2 3/3] Cygwin: pty: Fixup pty state after a cygwin app exits
Takashi Yano <[email protected]> Tue, 30 Jun 2026 17:36:59 +0900
| Newsgroups | gmane.os.cygwin.patches |
|---|---|
| Message-ID | <[email protected]> |
Hi Johannes,
The second and the third patches of your three additional patches LGTM.
Pushed to master.
Thanks!
I'm reviewing the first one. Please wait.
On Sat, 27 Jun 2026 09:18:41 +0200 (CEST)
Johannes Schindelin wrote:
> 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
> >
> >
--
Takashi Yano <[email protected]>