[PATCH] Cygwin: pty: Add missing CloseHandle() calls.

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

[PATCH] Cygwin: pty: Add missing CloseHandle() calls.

Takashi Yano
---
 winsup/cygwin/fhandler_tty.cc | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 65b12fd6c..23156f977 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2242,11 +2242,27 @@ fhandler_pty_master::close ()
   /* Terminate helper process */
   SetEvent (get_ttyp ()->h_helper_goodbye);
   WaitForSingleObject (get_ttyp ()->h_helper_process, INFINITE);
+  CloseHandle (get_ttyp ()->h_helper_goodbye);
+  CloseHandle (get_ttyp ()->h_helper_process);
   /* FIXME: Pseudo console can be accessed via its handle
      only in the process which created it. What else can we do? */
   if (master_pid_tmp == myself->pid)
-    /* Release pseudo console */
-    ClosePseudoConsole (get_pseudo_console ());
+    {
+      /* ClosePseudoConsole() seems to have a bug that one
+ internal handle remains opened. This causes handle leak.
+ This is a workaround for this problem. */
+      typedef struct
+      {
+ HANDLE hWritePipe;
+ HANDLE hConDrvReference;
+ HANDLE hConHostProcess;
+      } HPCON_INTERNAL;
+      HPCON_INTERNAL *hp = (HPCON_INTERNAL *) get_pseudo_console ();
+      HANDLE tmp = hp->hConHostProcess;
+      /* Release pseudo console */
+      ClosePseudoConsole (get_pseudo_console ());
+      CloseHandle (tmp);
+    }
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
  }
@@ -3212,6 +3228,8 @@ fhandler_pty_master::setup_pseudoconsole ()
   FALSE, EXTENDED_STARTUPINFO_PRESENT,
   NULL, NULL, &si_helper.StartupInfo, &pi_helper);
   WaitForSingleObject (hello, INFINITE);
+  CloseHandle (hello);
+  CloseHandle (pi_helper.hThread);
   /* Retrieve pseudo console handles */
   DWORD rLen;
   char buf[64];
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Cygwin: pty: Add missing CloseHandle() calls.

Corinna Vinschen-2
Hi Takashi,

On Jan  1 15:48, Takashi Yano wrote:

> ---
>  winsup/cygwin/fhandler_tty.cc | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index 65b12fd6c..23156f977 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -2242,11 +2242,27 @@ fhandler_pty_master::close ()
>    /* Terminate helper process */
>    SetEvent (get_ttyp ()->h_helper_goodbye);
>    WaitForSingleObject (get_ttyp ()->h_helper_process, INFINITE);
> +  CloseHandle (get_ttyp ()->h_helper_goodbye);
> +  CloseHandle (get_ttyp ()->h_helper_process);
>    /* FIXME: Pseudo console can be accessed via its handle
>       only in the process which created it. What else can we do? */
>    if (master_pid_tmp == myself->pid)
> -    /* Release pseudo console */
> -    ClosePseudoConsole (get_pseudo_console ());
> +    {
> +      /* ClosePseudoConsole() seems to have a bug that one
> + internal handle remains opened. This causes handle leak.
> + This is a workaround for this problem. */
> +      typedef struct
> +      {
> + HANDLE hWritePipe;
> + HANDLE hConDrvReference;
> + HANDLE hConHostProcess;
> +      } HPCON_INTERNAL;
Can you please move this struct to some header?  Even if it's not a
really close match, maybe ntdll.h works best, combined with a short
comment what this is about...


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

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

[PATCH v2] Cygwin: pty: Add missing CloseHandle() calls.

Takashi Yano
- PTY code which support pseudo console has a problem that causes
  handle leaks. Four of these are bug in pty code, and the other
  one seems to be a bug of Windows10. ClosePseudoConsole() fails
  to close one internal handle. This patch fixes the issue.
---
 winsup/cygwin/fhandler_tty.cc | 16 ++++++++++++++--
 winsup/cygwin/ntdll.h         | 13 +++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index ca4dc1c20..1a3bdc5ea 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -2224,11 +2224,21 @@ fhandler_pty_master::close ()
   /* Terminate helper process */
   SetEvent (get_ttyp ()->h_helper_goodbye);
   WaitForSingleObject (get_ttyp ()->h_helper_process, INFINITE);
+  CloseHandle (get_ttyp ()->h_helper_goodbye);
+  CloseHandle (get_ttyp ()->h_helper_process);
   /* FIXME: Pseudo console can be accessed via its handle
      only in the process which created it. What else can we do? */
   if (master_pid_tmp == myself->pid)
-    /* Release pseudo console */
-    ClosePseudoConsole (get_pseudo_console ());
+    {
+      /* ClosePseudoConsole() seems to have a bug that one
+ internal handle remains opened. This causes handle leak.
+ This is a workaround for this problem. */
+      HPCON_INTERNAL *hp = (HPCON_INTERNAL *) get_pseudo_console ();
+      HANDLE tmp = hp->hConHostProcess;
+      /* Release pseudo console */
+      ClosePseudoConsole (get_pseudo_console ());
+      CloseHandle (tmp);
+    }
   get_ttyp ()->switch_to_pcon_in = false;
   get_ttyp ()->switch_to_pcon_out = false;
  }
@@ -3191,6 +3201,8 @@ fhandler_pty_master::setup_pseudoconsole ()
   TRUE, EXTENDED_STARTUPINFO_PRESENT,
   NULL, NULL, &si_helper.StartupInfo, &pi_helper);
   WaitForSingleObject (hello, INFINITE);
+  CloseHandle (hello);
+  CloseHandle (pi_helper.hThread);
   /* Retrieve pseudo console handles */
   DWORD rLen;
   char buf[64];
diff --git a/winsup/cygwin/ntdll.h b/winsup/cygwin/ntdll.h
index 1c07d0255..8a08111db 100644
--- a/winsup/cygwin/ntdll.h
+++ b/winsup/cygwin/ntdll.h
@@ -1763,4 +1763,17 @@ extern "C"
     return status;
   }
 }
+
+/* This is for pseudo console workaround. ClosePseudoConsole()
+   seems to have a bug that one internal handle remains opend.
+   This causes handle leak. To close this handle, it is needed
+   to access internal of HPCON. HPCON_INTERNAL is defined for
+   this purpose. The structure of internal of HPCON is not
+   documented. Refer to: https://github.com/Biswa96/XConPty */
+typedef struct _HPCON_INTERNAL
+{
+  HANDLE hWritePipe;
+  HANDLE hConDrvReference;
+  HANDLE hConHostProcess;
+} HPCON_INTERNAL;
 #endif
--
2.21.0