[PATCH] Cygwin: pty: Fix the issue regarding open and close multiple PTYs.

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Cygwin: pty: Fix the issue regarding open and close multiple PTYs.

Takashi Yano
- If two PTYs are opened in the same process and the first one
  is closed, the helper process for the first PTY remains running.
  This patch fixes the issue.
---
 winsup/cygwin/fhandler_tty.cc         |  8 ++++----
 winsup/utils/cygwin-console-helper.cc | 15 ++++++++++++++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index f10f0fc61..65b12fd6c 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2233,8 +2233,7 @@ fhandler_pty_master::close ()
     termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
   if (!NT_SUCCESS (status))
     debug_printf ("NtQueryObject: %y", status);
-  else if (obi.HandleCount == (get_pseudo_console () ? 2 : 1))
-      /* Helper process has inherited one. */
+  else if (obi.HandleCount == 1)
     {
       termios_printf ("Closing last master of pty%d", get_minor ());
       /* Close Pseudo Console */
@@ -3202,14 +3201,15 @@ fhandler_pty_master::setup_pseudoconsole ()
   path_conv helper ("/bin/cygwin-console-helper.exe");
   size_t len = helper.get_wide_win32_path_len ();
   helper.get_wide_win32_path (cmd);
-  __small_swprintf (cmd + len, L" %p %p %p", hello, goodbye, hw);
+  __small_swprintf (cmd + len, L" %p %p %p %p",
+    hello, goodbye, hw, GetCurrentProcessId ());
   si_helper.StartupInfo.dwFlags = STARTF_USESTDHANDLES;
   si_helper.StartupInfo.hStdInput = NULL;
   si_helper.StartupInfo.hStdOutput = NULL;
   si_helper.StartupInfo.hStdError = NULL;
   PROCESS_INFORMATION pi_helper;
   CreateProcessW (NULL, cmd, &sec_none, &sec_none,
-  TRUE, EXTENDED_STARTUPINFO_PRESENT,
+  FALSE, EXTENDED_STARTUPINFO_PRESENT,
   NULL, NULL, &si_helper.StartupInfo, &pi_helper);
   WaitForSingleObject (hello, INFINITE);
   /* Retrieve pseudo console handles */
diff --git a/winsup/utils/cygwin-console-helper.cc b/winsup/utils/cygwin-console-helper.cc
index 66004bd15..6255fb93d 100644
--- a/winsup/utils/cygwin-console-helper.cc
+++ b/winsup/utils/cygwin-console-helper.cc
@@ -4,14 +4,24 @@ int
 main (int argc, char **argv)
 {
   char *end;
+  HANDLE parent = NULL;
   if (argc < 3)
     exit (1);
+  if (argc == 5)
+    parent = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
+  strtoull (argv[4], &end, 0));
   HANDLE h = (HANDLE) strtoull (argv[1], &end, 0);
+  if (parent)
+    DuplicateHandle (parent, h, GetCurrentProcess (), &h,
+     0, FALSE, DUPLICATE_SAME_ACCESS);
   SetEvent (h);
-  if (argc == 4) /* Pseudo console helper mode for PTY */
+  if (argc == 4 || argc == 5) /* Pseudo console helper mode for PTY */
     {
       SetConsoleCtrlHandler (NULL, TRUE);
       HANDLE hPipe = (HANDLE) strtoull (argv[3], &end, 0);
+      if (parent)
+ DuplicateHandle (parent, hPipe, GetCurrentProcess (), &hPipe,
+ 0, FALSE, DUPLICATE_SAME_ACCESS);
       char buf[64];
       sprintf (buf, "StdHandles=%p,%p\n",
        GetStdHandle (STD_INPUT_HANDLE),
@@ -21,6 +31,9 @@ main (int argc, char **argv)
       CloseHandle (hPipe);
     }
   h = (HANDLE) strtoull (argv[2], &end, 0);
+  if (parent)
+    DuplicateHandle (parent, h, GetCurrentProcess (), &h,
+     0, FALSE, DUPLICATE_SAME_ACCESS);
   WaitForSingleObject (h, INFINITE);
   exit (0);
 }
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Fix the issue regarding open and close multiple PTYs.

Corinna Vinschen-2
Hi Takashi,

On Jan  1 15:47, Takashi Yano wrote:

> - If two PTYs are opened in the same process and the first one
>   is closed, the helper process for the first PTY remains running.
>   This patch fixes the issue.
> ---
> [...]
> diff --git a/winsup/utils/cygwin-console-helper.cc b/winsup/utils/cygwin-console-helper.cc
> index 66004bd15..6255fb93d 100644
> --- a/winsup/utils/cygwin-console-helper.cc
> +++ b/winsup/utils/cygwin-console-helper.cc
> @@ -4,14 +4,24 @@ int
>  main (int argc, char **argv)
>  {
>    char *end;
> +  HANDLE parent = NULL;
>    if (argc < 3)
>      exit (1);
> +  if (argc == 5)
> +    parent = OpenProcess (PROCESS_DUP_HANDLE, FALSE,
> +  strtoull (argv[4], &end, 0));
>    HANDLE h = (HANDLE) strtoull (argv[1], &end, 0);
> +  if (parent)
> +    DuplicateHandle (parent, h, GetCurrentProcess (), &h,
> +     0, FALSE, DUPLICATE_SAME_ACCESS);
>    SetEvent (h);
> -  if (argc == 4) /* Pseudo console helper mode for PTY */
> +  if (argc == 4 || argc == 5) /* Pseudo console helper mode for PTY */
>      {
>        SetConsoleCtrlHandler (NULL, TRUE);
>        HANDLE hPipe = (HANDLE) strtoull (argv[3], &end, 0);
> +      if (parent)
> + DuplicateHandle (parent, hPipe, GetCurrentProcess (), &hPipe,
> + 0, FALSE, DUPLICATE_SAME_ACCESS);
>        char buf[64];
>        sprintf (buf, "StdHandles=%p,%p\n",
>         GetStdHandle (STD_INPUT_HANDLE),
> @@ -21,6 +31,9 @@ main (int argc, char **argv)
>        CloseHandle (hPipe);
>      }
>    h = (HANDLE) strtoull (argv[2], &end, 0);
> +  if (parent)
> +    DuplicateHandle (parent, h, GetCurrentProcess (), &h,
> +     0, FALSE, DUPLICATE_SAME_ACCESS);
I think it would be better if cygwin-console-helper closes the parent
handle at this point before waiting.  I have a bad feeling keeping the
PROCESS_DUP_HANDLE handle open for longer than necessary, after the
handle leakage we had in execve lately.

But then again, given that Cygwin uses EXTENDED_STARTUPINFO_PRESENT
anyway, wouldn't it makes sense to pass the required handles with
PROC_THREAD_ATTRIBUTE_HANDLE_LIST to avoid having to open the
parent with PROCESS_DUP_HANDLE?


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Fix the issue regarding open and close multiple PTYs.

Takashi Yano
Hi Corinna,

On Mon, 13 Jan 2020 16:49:52 +0100
Corinna Vinschen wrote:
> But then again, given that Cygwin uses EXTENDED_STARTUPINFO_PRESENT
> anyway, wouldn't it makes sense to pass the required handles with
> PROC_THREAD_ATTRIBUTE_HANDLE_LIST to avoid having to open the
> parent with PROCESS_DUP_HANDLE?

Thanks for the advice. I didn't know PROC_THREAD_ATTRIBUTE_HANDLE_LIST.
It is very good idea to use PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

I will submit a revised patch.

--
Takashi Yano <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

[PATCH v2] Cygwin: pty: Fix the issue regarding open and close multiple PTYs.

Takashi Yano
In reply to this post by Corinna Vinschen-2
- If two PTYs are opened in the same process and the first one
  is closed, the helper process for the first PTY remains running.
  This patch fixes the issue.
---
 winsup/cygwin/fhandler_tty.cc | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 1a3bdc5ea..042ffd19c 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2214,8 +2214,7 @@ fhandler_pty_master::close ()
     termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
   if (!NT_SUCCESS (status))
     debug_printf ("NtQueryObject: %y", status);
-  else if (obi.HandleCount == (get_pseudo_console () ? 2 : 1))
-      /* Helper process has inherited one. */
+  else if (obi.HandleCount == 1)
     {
       termios_printf ("Closing last master of pty%d", get_minor ());
       /* Close Pseudo Console */
@@ -3167,14 +3166,14 @@ fhandler_pty_master::setup_pseudoconsole ()
     get_ttyp ()->attach_pcon_in_fork = true;
 
   SIZE_T bytesRequired;
-  InitializeProcThreadAttributeList (NULL, 1, 0, &bytesRequired);
+  InitializeProcThreadAttributeList (NULL, 2, 0, &bytesRequired);
   STARTUPINFOEXW si_helper;
   ZeroMemory (&si_helper, sizeof (si_helper));
   si_helper.StartupInfo.cb = sizeof (STARTUPINFOEXW);
   si_helper.lpAttributeList = (PPROC_THREAD_ATTRIBUTE_LIST)
     HeapAlloc (GetProcessHeap (), 0, bytesRequired);
   InitializeProcThreadAttributeList (si_helper.lpAttributeList,
-     1, 0, &bytesRequired);
+     2, 0, &bytesRequired);
   UpdateProcThreadAttribute (si_helper.lpAttributeList,
      0,
      PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE,
@@ -3186,6 +3185,14 @@ fhandler_pty_master::setup_pseudoconsole ()
   /* Create a pipe for receiving pseudo console handles */
   HANDLE hr, hw;
   CreatePipe (&hr, &hw, &sec_none, 0);
+  /* Inherit only handles which are needed by helper. */
+  HANDLE handles_to_inherit[] = {hello, goodbye, hw};
+  UpdateProcThreadAttribute (si_helper.lpAttributeList,
+     0,
+     PROC_THREAD_ATTRIBUTE_HANDLE_LIST,
+     handles_to_inherit,
+     sizeof (handles_to_inherit),
+     NULL, NULL);
   /* Create helper process */
   WCHAR cmd[MAX_PATH];
   path_conv helper ("/bin/cygwin-console-helper.exe");
--
2.21.0