[PATCH] Cygwin: console: Avoid slipping past disable_master_thread check.

[PATCH] Cygwin: console: Avoid slipping past disable_master_thread check.

[Takashi Yano]

If disable_master_thread flag is set between the code checking that
flag not be set and the code acquiring input_mutex, input record is
processed once after setting disable_master_thread flag. This patch
prevents that.

Fixes: d4aacd50e6cf ("Cygwin: console: Add missing input_mutex guard.")
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
---
 winsup/cygwin/fhandler/console.cc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
index 6a42b4949..1c8d383cd 100644
--- a/winsup/cygwin/fhandler/console.cc
+++ b/winsup/cygwin/fhandler/console.cc
@@ -420,6 +420,12 @@ fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp)
 	}
 
       WaitForSingleObject (p->input_mutex, mutex_timeout);
+      /* Ensure accessing input recored is not disabled. */
+      if (con.disable_master_thread)
+	{
+	  ReleaseMutex (p->input_mutex);
+	  continue;
+	}
       total_read = 0;
       switch (cygwait (p->input_handle, (DWORD) 0))
 	{
@@ -4545,8 +4551,6 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
 	return;
     }
   const _minor_t unit = cons->get_minor ();
-  if (con.disable_master_thread == x)
-    return;
   cons->acquire_input_mutex (mutex_timeout);
   con.disable_master_thread = x;
   cons->release_input_mutex ();
-- 
2.43.0

Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check.

[Johannes Schindelin]

Hi Takashi,

On Sat, 3 Feb 2024, Takashi Yano wrote:

> If disable_master_thread flag is set between the code checking that
> flag not be set and the code acquiring input_mutex, input record is
> processed once after setting disable_master_thread flag. This patch
> prevents that.
>
> Fixes: d4aacd50e6cf ("Cygwin: console: Add missing input_mutex guard.")
> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>

Thank you for double-checking this (and for finding and fixing the bug).

I wonder what could be a symptom of this bug. I ask because we have
noticed a couple of inexplicable hangs in GitHub workflow runs in the Git
for Windows and the MSYS2 projects, hangs that are almost certainly due to
the ConPTY code in Cygwin, and I hope to figure out what causes them, and
even more importantly, how to fix those. Maybe the bug you fixed in this
patch could explain this?

Concretely, the hangs occur typically when some `pacman` process (a
package manager using the MSYS2 runtime, i.e. the Cygwin runtime with
several dozen patches on top) calls a few non-Cygwin processes. Those
processes seem to succeed, but there is an extra `pacman` process left
hanging around (reported using the same command-line as its parent
process, indicating a `fork()`ed child process or maybe that
signal-handler that is spawned for every non-Cygwin child process) and at
that point the program hangs indefinitely (or at least until the GitHub
workflow run times out after 6 hours).

I was not able to obtain any helpful stacktraces, they all seem to make no
sense, I only vaguely remember that one thread was waiting for an object,
but that could be a false flag.

Stopping those hanging `pacman` processes via `wmic process ... delete`
counter-intuitively fails to result in `pacman` to exit with a non-zero
exit code. Instead, the program now runs to completion successfully!

Most recently, these hangs became almost reliably reproducible, when we
started changing a certain GitHub workflow that runs on a Windows/ARM64
build agent. I suspect that Windows/ARM64 just makes this much more
likely, but that the bug is also there on regular Windows/x86_64.

What changed in the GitHub workflow is a new PowerShell script that runs
`pacman` a couple of times, trying to update packages, and after that
force-reinstalls a certain package for the benefit of its post-install
script. This post-install script is run using the (MSYS) Bash, and it
calls among other things (non-MSYS) `git.exe`. And that's where it hangs
almost every time.

When I log into the build agent via RDP, I do not see any `git.exe`
process running, but the same type of hanging `pacman` process as
indicated above. Using the `wmic` command to stop the hanging process lets
the GitHub workflow continue without any indication of failure.

Running the PowerShell script manually succeeds every single time, so I
think the hang might be connected to running things headlessly.

Do you have any idea what the bug could be? Or how I could diagnose this
better? Attaching via `gdb` only produces unhelpful stacktraces (that may
even be bogus, by the looks of it). Or do you think that your patch that I
am replying to could potentially fix this problem? How could the code be
improved to avoid those hangs altogether, or at least to make them easier
to diagnose?

Ciao,
Johannes

> ---
>  winsup/cygwin/fhandler/console.cc | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
> index 6a42b4949..1c8d383cd 100644
> --- a/winsup/cygwin/fhandler/console.cc
> +++ b/winsup/cygwin/fhandler/console.cc
> @@ -420,6 +420,12 @@ fhandler_console::cons_master_thread (handle_set_t *p, tty *ttyp)
>  	}
>
>        WaitForSingleObject (p->input_mutex, mutex_timeout);
> +      /* Ensure accessing input recored is not disabled. */
> +      if (con.disable_master_thread)
> +	{
> +	  ReleaseMutex (p->input_mutex);
> +	  continue;
> +	}
>        total_read = 0;
>        switch (cygwait (p->input_handle, (DWORD) 0))
>  	{
> @@ -4545,8 +4551,6 @@ fhandler_console::set_disable_master_thread (bool x, fhandler_console *cons)
>  	return;
>      }
>    const _minor_t unit = cons->get_minor ();
> -  if (con.disable_master_thread == x)
> -    return;
>    cons->acquire_input_mutex (mutex_timeout);
>    con.disable_master_thread = x;
>    cons->release_input_mutex ();
> --
> 2.43.0
>
>

Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check.

[Takashi Yano]

On Sat, 3 Feb 2024 15:53:29 +0100 (CET)
Johannes Schindelin wrote:
> I wonder what could be a symptom of this bug. I ask because we have
> noticed a couple of inexplicable hangs in GitHub workflow runs in the Git
> for Windows and the MSYS2 projects, hangs that are almost certainly due to
> the ConPTY code in Cygwin, and I hope to figure out what causes them, and
> even more importantly, how to fix those. Maybe the bug you fixed in this
> patch could explain this?

I don't think so. The symptom of this bug is that console input for
non-cygwin process can be lost or reordered at the starting timing
of that program for about 40 msec.

> Concretely, the hangs occur typically when some `pacman` process (a
> package manager using the MSYS2 runtime, i.e. the Cygwin runtime with
> several dozen patches on top) calls a few non-Cygwin processes. Those
> processes seem to succeed, but there is an extra `pacman` process left
> hanging around (reported using the same command-line as its parent
> process, indicating a `fork()`ed child process or maybe that
> signal-handler that is spawned for every non-Cygwin child process) and at
> that point the program hangs indefinitely (or at least until the GitHub
> workflow run times out after 6 hours).
> 
> I was not able to obtain any helpful stacktraces, they all seem to make no
> sense, I only vaguely remember that one thread was waiting for an object,
> but that could be a false flag.
> 
> Stopping those hanging `pacman` processes via `wmic process ... delete`
> counter-intuitively fails to result in `pacman` to exit with a non-zero
> exit code. Instead, the program now runs to completion successfully!
> 
> Most recently, these hangs became almost reliably reproducible, when we
> started changing a certain GitHub workflow that runs on a Windows/ARM64
> build agent. I suspect that Windows/ARM64 just makes this much more
> likely, but that the bug is also there on regular Windows/x86_64.
> 
> What changed in the GitHub workflow is a new PowerShell script that runs
> `pacman` a couple of times, trying to update packages, and after that
> force-reinstalls a certain package for the benefit of its post-install
> script. This post-install script is run using the (MSYS) Bash, and it
> calls among other things (non-MSYS) `git.exe`. And that's where it hangs
> almost every time.
> 
> When I log into the build agent via RDP, I do not see any `git.exe`
> process running, but the same type of hanging `pacman` process as
> indicated above. Using the `wmic` command to stop the hanging process lets
> the GitHub workflow continue without any indication of failure.
> 
> Running the PowerShell script manually succeeds every single time, so I
> think the hang might be connected to running things headlessly.
> 
> Do you have any idea what the bug could be? Or how I could diagnose this
> better? Attaching via `gdb` only produces unhelpful stacktraces (that may
> even be bogus, by the looks of it). Or do you think that your patch that I
> am replying to could potentially fix this problem? How could the code be
> improved to avoid those hangs altogether, or at least to make them easier
> to diagnose?

Thanks for details of your problem. Unfortunately, I have currently no idea.
Non-cygwin process is now treated very different from cygwin process because
it of course does not use cygwin functionality. For example:
(1) Cygwin setups pseudo-console for non-cygwin process in pty. Because pty is
    just a pipe for non-cygwin program and some programs do not work correctly
    on the pipe.
(2) Cygwin restores the default console mode for non-cygwin process if it is
    running in console. This is because, cygwin uses xterm compatible mode
    for cygwin process which is enabled by ENABLE_VIRTUAL_TYERMINAL_PROCESSING
    and ENABLE_VIRTUAL_TERMINAL_INPUT.
However, less cygwin users runs non-cygwin process in cygwin than MSYS2 users.
MSYS2 users uses non-MSYS2 program under MSYS2 environment on a daily basis,
so probability to hit a bug of (1) and (2) is relatively higher than cygwin
users.

If you attached gdb to non-msys2 program and failed to retrieve meaningfull
information, how about attaching gdb to the stub process? Note that the stub
process is hidden from normal cygwin tools such as 'ps'. However, it is real
parent process of the non-msys2 program and you can find it in taskmanager.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>

Re: [PATCH] Cygwin: console: Avoid slipping past disable_master_thread check.

[Jeremy Drake]

Thanks for taking the time to write this up, this issue has been bugging
me for years... (see also:
https://cygwin.com/pipermail/cygwin-patches/2021q4/011638.html)

On Sat, 3 Feb 2024, Johannes Schindelin wrote:

> Concretely, the hangs occur typically when some `pacman` process (a
> package manager using the MSYS2 runtime, i.e. the Cygwin runtime with
> several dozen patches on top) calls a few non-Cygwin processes. Those
> processes seem to succeed, but there is an extra `pacman` process left
> hanging around (reported using the same command-line as its parent
> process, indicating a `fork()`ed child process or maybe that
> signal-handler that is spawned for every non-Cygwin child process) and at
> that point the program hangs indefinitely (or at least until the GitHub
> workflow run times out after 6 hours).

I don't think this requires running non-Cygwin children - I see this most
often when pacman is using GPGME to validate signatueres.  That
fork/exec's (Cygwin) gpg.

> I was not able to obtain any helpful stacktraces, they all seem to make no
> sense, I only vaguely remember that one thread was waiting for an object,
> but that could be a false flag.

My recollection when I tried to debug was that every debugger I tried got
an error trying to get the context of the main thread of the hung process.
There was another thread, which seemed to be for Cygwin signal handling,
which was blithely waiting for an object, but I kind of expect that's what
it was supposed to be doing.

> Stopping those hanging `pacman` processes via `wmic process ... delete`
> counter-intuitively fails to result in `pacman` to exit with a non-zero
> exit code. Instead, the program now runs to completion successfully!

> Do you have any idea what the bug could be? Or how I could diagnose this
> better? Attaching via `gdb` only produces unhelpful stacktraces (that may
> even be bogus, by the looks of it). Or do you think that your patch that I
> am replying to could potentially fix this problem? How could the code be
> improved to avoid those hangs altogether, or at least to make them easier
> to diagnose?