Hi Takashi,
On 6/13/2026 7:07 AM, Takashi Yano wrote:
Currently, when a non-cygwin program (A) is about to exit, and another
non-cygwin program (B) is started, input transferring between cyg-pipe
and nan-pipe may not work as expected. When the non-cygwin program (A)
^^^
exits, input transferring from nat-pipe to cyg-pipe will be performed.
However, the the non-cygwin program (B) will performs input transferring
from cyg-pipe to nat-pipe at the same time.
I'm having some trouble understanding whether the above block all
describes past (before patch) operation, or whether it shifts in the
middle to discuss after patch operation.
Also, is there a simple example of this situation you could add? Is the
case of a (Cygwin) shell launching two foreground Windows programs one
after the other a possible example?
1) The the non-cygwin program (A) checks current input pipe state,
then it is nat-pip since the this program is a non-cygwin program.
^^^^^^^
The program (A) also checkes if any handover target exists, but
^^^^^^^
it is not found since the probram (B) is not started yet. So,
^^^^^^^
the program (A) decided to transfer input form nat-pipe to cyg-
pipe.
2) Before the non-cygwin (A) program performs input transferring,
if the non-cygwin program (B) is started and checks the input
pipe state, it is nat-pipe state, so the non-cygwin program (B)
does not perform input transferring.
3) However, just after that, the non-cygwin program (A) performs
input transferring from nat-pipe to cyg-pipe, so typeahead input
will be stored in cyg-pipe.
4) The non-cygwin program (B) cannot read the typeahead input
because it is now in the cyg-pipe.
Maybe I do understand now. The above is a sequence of steps to
demonstrate the issue being fixed. Am I correct? And the below
describes root cause and how it is fixed?
Transferring input itself is guarded by input_mutex, but the pre-
check is not. With this patch, the guard is enhanced so that the
state check and tranferring input are done in atomic way.
^^^^^^^^^^^
Fixes: f20641789427 ("Cygwin: pty: Reduce unecessary input transfer.")
^^^^^^^^^^ do not correct..
if same in original
Signed-off-by: Takashi Yano <[email protected]>
Reviewed-by:
---
v2: Guard term_has_pcon_cap() as well
v3: Acquire pipe_sw_mutex first before acquiring input_mutex
v4: Don't call to_be_read_from_nat_pipe() while holding input_mutex
(This simplifies the to_be_read_from_nat_pipe())
v5: Don't wait pcon_handle_ready_event in req_xfer_input is set.
In pcon_start mode, send input data except for response to CSI6n
and CSI c to nat-pipe (to_slave_nat) rather than line_edit() if
pipe state is tty::to_nat (this happens when req_xfer_input mode).
winsup/cygwin/fhandler/pty.cc | 110 ++++++++++++++++++----------------
1 file changed, 57 insertions(+), 53 deletions(-)
diff --git a/winsup/cygwin/fhandler/pty.cc b/winsup/cygwin/fhandler/pty.cc
index e0fc67ae1..17bef7ea4 100644
--- a/winsup/cygwin/fhandler/pty.cc
+++ b/winsup/cygwin/fhandler/pty.cc
@@ -657,8 +657,7 @@ fhandler_pty_master::accept_input ()
HANDLE write_to = get_output_handle ();
tmp_pathbuf tp;
- if (to_be_read_from_nat_pipe ()
- && get_ttyp ()->pty_input_state == tty::to_nat)
+ if (get_ttyp ()->pty_input_state == tty::to_nat)
{
/* This code is reached if non-cygwin app is foreground and
pseudo console is not enabled. */
@@ -1274,18 +1273,18 @@ fhandler_pty_slave::reset_switch_to_nat_pipe (void)
mutex_timeout = INFINITE;
if (isHybrid)
{
+ WaitForSingleObject (input_mutex, mutex_timeout);
if (get_ttyp ()->getpgid () == myself->pgid
&& GetStdHandle (STD_INPUT_HANDLE) == get_handle ()
&& get_ttyp ()->pty_input_state_eq (tty::to_nat))
{
- WaitForSingleObject (input_mutex, mutex_timeout);
acquire_attach_mutex (mutex_timeout);
transfer_input (tty::to_cyg, get_handle_nat (), get_ttyp (),
input_available_event,
input_transferred_to_cyg);
release_attach_mutex ();
- ReleaseMutex (input_mutex);
}
+ ReleaseMutex (input_mutex);
if (get_ttyp ()->master_is_running_as_service
&& get_ttyp ()->pcon_activated)
/* If the master is running as service, re-attaching to
@@ -1452,19 +1451,10 @@ fhandler_pty_slave::mask_switch_to_nat_pipe (bool mask,
bool xfer)
bool
fhandler_pty_common::to_be_read_from_nat_pipe (void)
{
- /* If the slave is in setup_pseudoconsole(), pipe_sw_mutex cannot
- be acquired because the slave has it. In this case pcon_start
- will be asserted. During pcon_start, other input than response
- to CSI6n should be go to cyg-pipe. So, wait for pcon_start and
- return false. */
- while (WaitForSingleObject (pipe_sw_mutex, 0) == WAIT_TIMEOUT)
- if (get_ttyp ()->pcon_start || get_ttyp ()->pcon_start_csi_c
- || get_ttyp ()->pcon_start_pid)
- return false;
- else
- yield ();
-
bool ret = false;
+
+ WaitForSingleObject (pipe_sw_mutex, INFINITE);
+
I am a little concerned if the replacement WFSO is equivalent to the
looping WFSO being replaced. I.e., it terminates for the same
condition(s) with the pty being in correct state. I can't point to
something specific though. Can you reassure me? Or is this just
re-establishing code to the way it was before?
if (!get_ttyp ()->switch_to_nat_pipe)
goto out;
@@ -2383,6 +2373,26 @@ fhandler_pty_master::write (const void *ptr, size_t len)
int pcon_start_mode =
get_ttyp ()->pcon_start ? 1 : (get_ttyp ()->pcon_start_csi_c ? 2 : 0);
+
+ /* This input transfer is needed when cygwin-app which is started from
+ non-cygwin app is terminated if pseudo console is disabled. */
^^ maybe change "if" to "while" here
+ if (!get_ttyp ()->pcon_activated && !pcon_start_mode
+ && to_be_read_from_nat_pipe ())
+ {
+ WaitForSingleObject (input_mutex, mutex_timeout);
+ if (get_ttyp ()->nat_fg (get_ttyp ()->getpgid ())
+ && get_ttyp ()->pty_input_state == tty::to_cyg)
+ {
+ acquire_attach_mutex (mutex_timeout);
+ fhandler_pty_slave::transfer_input (tty::to_nat, from_master,
+ get_ttyp (),
+ input_available_event,
+ input_transferred_to_cyg);
+ release_attach_mutex ();
+ }
+ ReleaseMutex (input_mutex);
+ }
+
if (pcon_start_mode)
{ /* Reaches here when pseudo console initialization is on going. */
/* Pseudo condole support uses "CSI6n" to get cursor position.
@@ -2404,7 +2414,13 @@ fhandler_pty_master::write (const void *ptr, size_t len)
if (p[i] == '\033')
{
if (ixput)
- line_edit (wpbuf, ixput, ti, &ret);
+ {
+ if (get_ttyp ()->req_xfer_input
+ && get_ttyp ()->pty_input_state_eq (tty::to_nat))
+ WriteFile (to_slave_nat, wpbuf, ixput, &n, NULL);
+ else
+ line_edit (wpbuf, ixput, ti, &ret);
+ }
ixput = 0;
state = 1;
wp_tid = _my_tls.thread_id;
@@ -2422,7 +2438,13 @@ fhandler_pty_master::write (const void *ptr, size_t len)
}
}
else
- line_edit (p + i, 1, ti, &ret);
+ {
+ if (get_ttyp ()->req_xfer_input
+ && get_ttyp ()->pty_input_state_eq (tty::to_nat))
+ WriteFile (to_slave_nat, p + i, 1, &n, NULL);
+ else
+ line_edit (p + i, 1, ti, &ret);
+ }
It is unfortunate we have the two line_edit() calls above now being
wrapped in almost identical fashion in two different locations. I can't
think of a way to pretty this, other than adding a couple of parameters
to line_edit() and changing all the other calling sites to pass zeroes.
Maybe you have another idea? But no need to spend much time on it.
len = orig_len - i - 1;
ptr = p + i + 1;
if (state == 1 && wp_tid == _my_tls.thread_id && p[i] == 'R')
@@ -2454,6 +2476,7 @@ fhandler_pty_master::write (const void *ptr, size_t len)
if (pcon_start_mode
&& !get_ttyp ()->pcon_start && !get_ttyp ()->pcon_start_csi_c)
{ /* Pseudo console initialization has been done in above code. */
+ WaitForSingleObject (input_mutex, mutex_timeout);
pinfo pp (get_ttyp ()->pcon_start_pid);
if (get_ttyp ()->switch_to_nat_pipe
&& pp && pp->pgid == get_ttyp ()->getpgid ()
@@ -2463,8 +2486,9 @@ fhandler_pty_master::write (const void *ptr, size_t len)
{
HANDLE pcon_handle_ready_event =
get_ttyp ()->pcon_handle_ready_event;
- get_handle_from_process (get_ttyp ()->nat_pipe_owner_pid,
- pcon_handle_ready_event);
+ pcon_handle_ready_event =
+ get_handle_from_process (get_ttyp ()->nat_pipe_owner_pid,
+ pcon_handle_ready_event);
if (pcon_handle_ready_event)
{
cygwait (pcon_handle_ready_event, INFINITE);
@@ -2475,7 +2499,6 @@ fhandler_pty_master::write (const void *ptr, size_t len)
/* This accept_input() call is needed in order to transfer input
which is not accepted yet to non-cygwin pipe. */
- WaitForSingleObject (input_mutex, mutex_timeout);
if (get_readahead_valid ())
accept_input ();
acquire_attach_mutex (mutex_timeout);
@@ -2484,9 +2507,9 @@ fhandler_pty_master::write (const void *ptr, size_t len)
input_available_event,
input_transferred_to_cyg);
release_attach_mutex ();
- ReleaseMutex (input_mutex);
}
get_ttyp ()->req_xfer_input = false;
+ ReleaseMutex (input_mutex);
get_ttyp ()->pcon_start_pid = 0;
}
if (len == 0)
@@ -2496,7 +2519,7 @@ fhandler_pty_master::write (const void *ptr, size_t len)
/* Write terminal input to to_slave_nat pipe instead of output_handle
if current application is native console application. */
WaitForSingleObject (input_mutex, mutex_timeout);
- if (to_be_read_from_nat_pipe () && get_ttyp ()->pcon_activated
+ if (get_ttyp ()->pcon_activated
&& get_ttyp ()->pty_input_state == tty::to_nat)
{ /* Reaches here when non-cygwin app is foreground and pseudo console
is activated. */
@@ -2580,20 +2603,6 @@ fhandler_pty_master::write (const void *ptr, size_t len)
/* The code path reaches here when pseudo console is not activated
or cygwin process is foreground even though pseudo console is
activated. */
-
- /* This input transfer is needed when cygwin-app which is started from
- non-cygwin app is terminated if pseudo console is disabled. */
- if (to_be_read_from_nat_pipe () && !get_ttyp ()->pcon_activated
- && get_ttyp ()->nat_fg (get_ttyp ()->getpgid ())
- && get_ttyp ()->pty_input_state == tty::to_cyg)
- {
- acquire_attach_mutex (mutex_timeout);
- fhandler_pty_slave::transfer_input (tty::to_nat, from_master,
- get_ttyp (), input_available_event,
- input_transferred_to_cyg);
- release_attach_mutex ();
- }
-
line_edit_status status = line_edit (p, len, ti, &ret);
ReleaseMutex (input_mutex);
@@ -4537,9 +4546,9 @@ fhandler_pty_slave::setup_for_non_cygwin_app (bool nopcon,
const WCHAR *envblock,
bool stdin_is_ptys)
{
+ WaitForSingleObject (pipe_sw_mutex, INFINITE);
if (disable_pcon || !term_has_pcon_cap (envblock))
nopcon = true;
- WaitForSingleObject (pipe_sw_mutex, INFINITE);
/* Setting switch_to_nat_pipe is necessary even if pseudo console
will not be activated. */
fhandler_base *fh = ::cygheap->fdtab[0];
@@ -4555,16 +4564,16 @@ fhandler_pty_slave::setup_for_non_cygwin_app (bool
nopcon,
pcon_enabled = setup_pseudoconsole ();
ReleaseMutex (pipe_sw_mutex);
/* For pcon enabled case, transfer_input() is called in master::write() */
+ WaitForSingleObject (input_mutex, mutex_timeout);
if (!pcon_enabled && get_ttyp ()->getpgid () == myself->pgid
&& stdin_is_ptys && get_ttyp ()->pty_input_state_eq (tty::to_cyg))
{
- WaitForSingleObject (input_mutex, mutex_timeout);
acquire_attach_mutex (mutex_timeout);
transfer_input (tty::to_nat, get_handle (), get_ttyp (),
input_available_event, input_transferred_to_cyg);
release_attach_mutex ();
- ReleaseMutex (input_mutex);
}
+ ReleaseMutex (input_mutex);
}
void
@@ -4573,22 +4582,22 @@ fhandler_pty_slave::cleanup_for_non_cygwin_app
(handle_set_t *p, tty *ttyp,
DWORD force_switch_to)
{
ttyp->wait_fwd ();
+ WaitForSingleObject (p->pipe_sw_mutex, INFINITE);
+ WaitForSingleObject (p->input_mutex, mutex_timeout);
if (nat_pipe_owner_self (ttyp->nat_pipe_owner_pid))
{
DWORD switch_to = get_winpid_to_hand_over (ttyp, force_switch_to);
if ((!switch_to && (ttyp->pcon_activated || stdin_is_ptys))
&& ttyp->pty_input_state_eq (tty::to_nat))
{
- WaitForSingleObject (p->input_mutex, mutex_timeout);
acquire_attach_mutex (mutex_timeout);
transfer_input (tty::to_cyg, p->from_master_nat, ttyp,
p->input_available_event,
p->input_transferred_to_cyg);
release_attach_mutex ();
- ReleaseMutex (p->input_mutex);
}
}
- WaitForSingleObject (p->pipe_sw_mutex, INFINITE);
+ ReleaseMutex (p->input_mutex);
if (ttyp->pcon_activated)
close_pseudoconsole (ttyp, force_switch_to);
else
@@ -4602,27 +4611,23 @@ fhandler_pty_slave::setpgid_aux (pid_t pid)
reset_switch_to_nat_pipe ();
WaitForSingleObject (pipe_sw_mutex, INFINITE);
+ WaitForSingleObject (input_mutex, mutex_timeout);
bool was_nat_fg = get_ttyp ()->nat_fg (tc ()->pgid);
bool nat_fg = get_ttyp ()->nat_fg (pid);
if (!was_nat_fg && nat_fg && get_ttyp ()->switch_to_nat_pipe
&& get_ttyp ()->pty_input_state_eq (tty::to_cyg))
{
- ReleaseMutex (pipe_sw_mutex);
- WaitForSingleObject (input_mutex, mutex_timeout);
acquire_attach_mutex (mutex_timeout);
transfer_input (tty::to_nat, get_handle (), get_ttyp (),
input_available_event, input_transferred_to_cyg);
release_attach_mutex ();
- ReleaseMutex (input_mutex);
}
else if (was_nat_fg && !nat_fg && get_ttyp ()->switch_to_nat_pipe
&& get_ttyp ()->pty_input_state_eq (tty::to_nat))
{
- ReleaseMutex (pipe_sw_mutex);
bool attach_restore = false;
HANDLE from = get_handle_nat ();
DWORD resume_pid = 0;
- WaitForSingleObject (input_mutex, mutex_timeout);
if (get_ttyp ()->pcon_activated && get_ttyp ()->nat_pipe_owner_pid
&& !get_console_process_id (get_ttyp ()->nat_pipe_owner_pid, true))
{
@@ -4640,10 +4645,9 @@ fhandler_pty_slave::setpgid_aux (pid_t pid)
resume_from_temporarily_attach (resume_pid);
else
release_attach_mutex ();
- ReleaseMutex (input_mutex);
}
- else
- ReleaseMutex (pipe_sw_mutex);
+ ReleaseMutex (input_mutex);
+ ReleaseMutex (pipe_sw_mutex);
}
bool
@@ -4653,8 +4657,8 @@ fhandler_pty_master::need_send_ctrl_c_event ()
apps will be done in pseudo console, therefore, sending it in
fhandler_pty_master::write() duplicates that event for non-cygwin
apps. So return false if pseudo console is activated. */
- return !(to_be_read_from_nat_pipe () && get_ttyp ()->pcon_activated
- && get_ttyp ()->pty_input_state == tty::to_nat);
+ return !(get_ttyp ()->pcon_activated
+ && get_ttyp ()->pty_input_state == tty::to_nat);
}
void
The rest LGTM. Let me know what you think about my comments.
..mark
