[PATCH 0/2] Fix race issues.

[PATCH 0/2] Fix race issues.

[Takashi Yano]

Takashi Yano (2):
  Cygwin: console: Fix race issue regarding cons_master_thread().
  Cygwin: pty: Fix race issue in inheritance of pseudo console.

 winsup/cygwin/fhandler_console.cc |  10 ++-
 winsup/cygwin/fhandler_tty.cc     | 108 ++++++++++++++++++------------
 winsup/cygwin/tty.cc              |  15 ++---
 winsup/cygwin/tty.h               |   2 +-
 4 files changed, 77 insertions(+), 58 deletions(-)

-- 
2.31.1

[PATCH 1/2] Cygwin: console: Fix race issue regarding cons_master_thread().

[Takashi Yano]

- With this patch, the race issue regarding starting/stopping
  cons_master_thread() introduced by commit ff4440fc is fixed.

  Addresses:
    https://cygwin.com/pipermail/cygwin/2021-April/248292.html
---
 winsup/cygwin/fhandler_console.cc | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 0b33a1370..e418aac97 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -48,6 +48,7 @@ details. */
 #define con_is_legacy (shared_console_info && con.is_legacy)
 
 #define CONS_THREAD_SYNC "cygcons.thread_sync"
+static bool NO_COPY master_thread_started = false;
 
 const unsigned fhandler_console::MAX_WRITE_CHARS = 16384;
 
@@ -184,6 +185,7 @@ cons_master_thread (VOID *arg)
 		   GetCurrentProcess (), &thread_sync_event,
 		   0, FALSE, DUPLICATE_SAME_ACCESS);
   SetEvent (thread_sync_event);
+  master_thread_started = true;
   /* Do not touch class members after here because the class instance
      may have been destroyed. */
   fhandler_console::cons_master_thread (&handle_set, ttyp);
@@ -370,6 +372,8 @@ fhandler_console::set_unit ()
     }
   if (!created && shared_console_info)
     {
+      while (con.owner > MAX_PID)
+	Sleep (1);
       pinfo p (con.owner);
       if (!p)
 	con.owner = myself->pid;
@@ -1393,14 +1397,16 @@ fhandler_console::close ()
 
   release_output_mutex ();
 
-  if (shared_console_info && con.owner == myself->pid)
+  if (shared_console_info && con.owner == myself->pid
+      && master_thread_started)
     {
       char name[MAX_PATH];
       shared_name (name, CONS_THREAD_SYNC, get_minor ());
       thread_sync_event = OpenEvent (MAXIMUM_ALLOWED, FALSE, name);
-      con.owner = 0;
+      con.owner = MAX_PID + 1;
       WaitForSingleObject (thread_sync_event, INFINITE);
       CloseHandle (thread_sync_event);
+      con.owner = 0;
     }
 
   CloseHandle (input_mutex);
-- 
2.31.1

[PATCH 2/2] Cygwin: pty: Fix race issue in inheritance of pseudo console.

[Takashi Yano]

- If multiple non-cygwin processes are started/ended simultaneously,
  inheritance of pseudo console sometimes fails. This patch fixes
  the issue.

  Addresses:
    https://cygwin.com/pipermail/cygwin/2021-April/248292.html
---
 winsup/cygwin/fhandler_tty.cc | 108 ++++++++++++++++++++--------------
 winsup/cygwin/tty.cc          |  15 ++---
 winsup/cygwin/tty.h           |   2 +-
 3 files changed, 69 insertions(+), 56 deletions(-)

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index 73aec2ade..ba9f4117f 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -1072,6 +1072,24 @@ fhandler_pty_slave::set_switch_to_pcon (void)
     }
 }
 
+inline static bool
+pcon_pid_alive (DWORD pid)
+{
+  if (pid == 0)
+    return false;
+  HANDLE h = OpenProcess (SYNCHRONIZE, FALSE, pid);
+  if (h == NULL)
+    return false;
+  CloseHandle (h);
+  return true;
+}
+
+inline static bool
+pcon_pid_self (DWORD pid)
+{
+  return (pid == myself->exec_dwProcessId);
+}
+
 void
 fhandler_pty_slave::reset_switch_to_pcon (void)
 {
@@ -1153,17 +1171,8 @@ fhandler_pty_slave::reset_switch_to_pcon (void)
   if (isHybrid)
     return;
   WaitForSingleObject (pcon_mutex, INFINITE);
-  HANDLE h;
-  if (get_ttyp ()->pcon_pid > MAX_PID &&
-      (h = OpenProcess (SYNCHRONIZE, FALSE, get_ttyp ()->pcon_pid - MAX_PID)))
-    {
-      /* There is a process which is grabbing pseudo console. */
-      CloseHandle (h);
-      ReleaseMutex (pcon_mutex);
-      return;
-    }
-  if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
-      && !!pinfo (get_ttyp ()->pcon_pid))
+  if (!pcon_pid_self (get_ttyp ()->pcon_pid)
+      && pcon_pid_alive (get_ttyp ()->pcon_pid))
     {
       /* There is a process which is grabbing pseudo console. */
       ReleaseMutex (pcon_mutex);
@@ -1975,19 +1984,15 @@ fhandler_pty_common::resize_pseudo_console (struct winsize *ws)
   COORD size;
   size.X = ws->ws_col;
   size.Y = ws->ws_row;
-  pinfo p (get_ttyp ()->pcon_pid);
-  if (p)
-    {
-      HPCON_INTERNAL hpcon_local;
-      HANDLE pcon_owner =
-	OpenProcess (PROCESS_DUP_HANDLE, FALSE, p->exec_dwProcessId);
-      DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_write_pipe,
-		       GetCurrentProcess (), &hpcon_local.hWritePipe,
-		       0, TRUE, DUPLICATE_SAME_ACCESS);
-      ResizePseudoConsole ((HPCON) &hpcon_local, size);
-      CloseHandle (pcon_owner);
-      CloseHandle (hpcon_local.hWritePipe);
-    }
+  HPCON_INTERNAL hpcon_local;
+  HANDLE pcon_owner =
+    OpenProcess (PROCESS_DUP_HANDLE, FALSE, get_ttyp ()->pcon_pid);
+  DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_write_pipe,
+		   GetCurrentProcess (), &hpcon_local.hWritePipe,
+		   0, TRUE, DUPLICATE_SAME_ACCESS);
+  ResizePseudoConsole ((HPCON) &hpcon_local, size);
+  CloseHandle (pcon_owner);
+  CloseHandle (hpcon_local.hWritePipe);
 }
 
 void
@@ -3085,9 +3090,8 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
     {
       fhandler_pty_slave *ptys = (fhandler_pty_slave *) fh;
       ptys->get_ttyp ()->switch_to_pcon_in = true;
-      if (ptys->get_ttyp ()->pcon_pid == 0
-	  || !pinfo (ptys->get_ttyp ()->pcon_pid))
-	ptys->get_ttyp ()->pcon_pid = myself->pid;
+      if (!pcon_pid_alive (ptys->get_ttyp ()->pcon_pid))
+	ptys->get_ttyp ()->pcon_pid = myself->exec_dwProcessId;
     }
 
   if (nopcon)
@@ -3107,8 +3111,7 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
     }
 
   HANDLE hpConIn, hpConOut;
-  if (get_ttyp ()->pcon_pid && get_ttyp ()->pcon_pid != myself->pid
-      && !!pinfo (get_ttyp ()->pcon_pid) && get_ttyp ()->pcon_activated)
+  if (get_ttyp ()->pcon_activated)
     {
       if (GetStdHandle (STD_INPUT_HANDLE) == get_handle ())
 	{ /* Send CSI6n just for requesting transfer input. */
@@ -3119,11 +3122,14 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
 	  get_ttyp ()->pcon_start_pid = myself->pid;
 	  WriteFile (get_output_handle (), "\033[6n", 4, &n, NULL);
 	  ReleaseMutex (input_mutex);
+	  while (get_ttyp ()->pcon_start)
+	    Sleep (1);
 	}
       /* Attach to the pseudo console which already exits. */
-      pinfo p (get_ttyp ()->pcon_pid);
       HANDLE pcon_owner =
-	OpenProcess (PROCESS_DUP_HANDLE, FALSE, p->exec_dwProcessId);
+	OpenProcess (PROCESS_DUP_HANDLE, FALSE, get_ttyp ()->pcon_pid);
+      if (pcon_owner == NULL)
+	return false;
       DuplicateHandle (pcon_owner, get_ttyp ()->h_pcon_in,
 		       GetCurrentProcess (), &hpConIn,
 		       0, TRUE, DUPLICATE_SAME_ACCESS);
@@ -3132,7 +3138,7 @@ fhandler_pty_slave::setup_pseudoconsole (bool nopcon)
 		       0, TRUE, DUPLICATE_SAME_ACCESS);
       CloseHandle (pcon_owner);
       FreeConsole ();
-      AttachConsole (p->exec_dwProcessId);
+      AttachConsole (get_ttyp ()->pcon_pid);
       goto skip_create;
     }
 
@@ -3287,10 +3293,10 @@ skip_create:
     }
   while (false);
 
-  if (get_ttyp ()->pcon_pid == 0 || !pinfo (get_ttyp ()->pcon_pid))
-    get_ttyp ()->pcon_pid = myself->pid;
+  if (!pcon_pid_alive (get_ttyp ()->pcon_pid))
+    get_ttyp ()->pcon_pid = myself->exec_dwProcessId;
 
-  if (hpcon && get_ttyp ()->pcon_pid == myself->pid)
+  if (hpcon && pcon_pid_self (get_ttyp ()->pcon_pid))
     {
       HPCON_INTERNAL *hp = (HPCON_INTERNAL *) hpcon;
       get_ttyp ()->h_pcon_write_pipe = hp->hWritePipe;
@@ -3381,15 +3387,14 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
   if (force_switch_to)
     {
       switch_to_stub = force_switch_to;
-      new_pcon_pid = force_switch_to + MAX_PID;
-      ttyp->setpgid (new_pcon_pid);
+      new_pcon_pid = force_switch_to;
+      ttyp->setpgid (force_switch_to + MAX_PID);
     }
-  else if (ttyp->pcon_pid == myself->pid)
+  else if (pcon_pid_self (ttyp->pcon_pid))
     {
       /* Search another process which attaches to the pseudo console */
       DWORD current_pid = myself->exec_dwProcessId ?: myself->dwProcessId;
-      switch_to =
-	get_console_process_id (current_pid, false, true);
+      switch_to = get_console_process_id (current_pid, false, true);
       if (switch_to)
 	{
 	  pinfo p (cygwin_pid (switch_to));
@@ -3397,15 +3402,21 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
 	    {
 	      if (p->exec_dwProcessId)
 		switch_to_stub = p->exec_dwProcessId;
-	      new_pcon_pid = p->pid;
+	      new_pcon_pid = p->exec_dwProcessId;
 	    }
 	}
+      else
+	{
+	  switch_to = get_console_process_id (current_pid, false, false);
+	  if (switch_to)
+	    new_pcon_pid = switch_to;
+	}
     }
   if (ttyp->pcon_activated)
     {
       ttyp->previous_code_page = GetConsoleCP ();
       ttyp->previous_output_code_page = GetConsoleOutputCP ();
-      if (ttyp->pcon_pid == myself->pid)
+      if (pcon_pid_self (ttyp->pcon_pid))
 	{
 	  switch_to = switch_to_stub ?: switch_to;
 	  if (switch_to)
@@ -3447,6 +3458,15 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
 	      ttyp->h_pcon_conhost_process = new_conhost_process;
 	      ttyp->h_pcon_in = new_pcon_in;
 	      ttyp->h_pcon_out = new_pcon_out;
+	      FreeConsole ();
+	      pinfo p (myself->ppid);
+	      if (p)
+		{
+		  if (!AttachConsole (p->dwProcessId))
+		    AttachConsole (ATTACH_PARENT_PROCESS);
+		}
+	      else
+		AttachConsole (ATTACH_PARENT_PROCESS);
 	    }
 	  else
 	    { /* Close pseudo console */
@@ -3462,7 +3482,7 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
 	      /* Reconstruct pseudo console handler container here for close */
 	      HPCON_INTERNAL *hp =
 		(HPCON_INTERNAL *) HeapAlloc (GetProcessHeap (), 0,
-					      sizeof (*hp));
+					      sizeof (HPCON_INTERNAL));
 	      hp->hWritePipe = ttyp->h_pcon_write_pipe;
 	      hp->hConDrvReference = ttyp->h_pcon_condrv_reference;
 	      hp->hConHostProcess = ttyp->h_pcon_conhost_process;
@@ -3489,7 +3509,7 @@ fhandler_pty_slave::close_pseudoconsole (tty *ttyp, DWORD force_switch_to)
 	    AttachConsole (ATTACH_PARENT_PROCESS);
 	}
     }
-  else if (ttyp->pcon_pid == myself->pid)
+  else if (pcon_pid_self (ttyp->pcon_pid))
     {
       if (switch_to_stub)
 	ttyp->pcon_pid = new_pcon_pid;
diff --git a/winsup/cygwin/tty.cc b/winsup/cygwin/tty.cc
index 96acde387..2566f4c45 100644
--- a/winsup/cygwin/tty.cc
+++ b/winsup/cygwin/tty.cc
@@ -327,25 +327,18 @@ tty_min::setpgid (int pid)
 	       && ttyp->pcon_input_state_eq (tty::to_nat))
 	{
 	  bool attach_restore = false;
-	  DWORD pcon_winpid = 0;
-	  if (ttyp->pcon_pid)
-	    {
-	      pinfo p (ttyp->pcon_pid);
-	      if (p)
-		pcon_winpid = p->exec_dwProcessId ?: p->dwProcessId;
-	    }
 	  HANDLE from = ptys->get_handle_nat ();
-	  if (ttyp->pcon_activated && pcon_winpid
-	      && !ptys->get_console_process_id (pcon_winpid, true))
+	  if (ttyp->pcon_activated && ttyp->pcon_pid
+	      && !ptys->get_console_process_id (ttyp->pcon_pid, true))
 	    {
 	      HANDLE pcon_owner =
-		OpenProcess (PROCESS_DUP_HANDLE, FALSE, pcon_winpid);
+		OpenProcess (PROCESS_DUP_HANDLE, FALSE, ttyp->pcon_pid);
 	      DuplicateHandle (pcon_owner, ttyp->h_pcon_in,
 			       GetCurrentProcess (), &from,
 			       0, TRUE, DUPLICATE_SAME_ACCESS);
 	      CloseHandle (pcon_owner);
 	      FreeConsole ();
-	      AttachConsole (pcon_winpid);
+	      AttachConsole (ttyp->pcon_pid);
 	      attach_restore = true;
 	    }
 	  WaitForSingleObject (ptys->input_mutex, INFINITE);
diff --git a/winsup/cygwin/tty.h b/winsup/cygwin/tty.h
index 12c926ec0..8479dd2ec 100644
--- a/winsup/cygwin/tty.h
+++ b/winsup/cygwin/tty.h
@@ -113,7 +113,7 @@ private:
   bool pcon_start;
   pid_t pcon_start_pid;
   bool switch_to_pcon_in;
-  pid_t pcon_pid;
+  DWORD pcon_pid;
   UINT term_code_page;
   DWORD pcon_last_time;
   HANDLE h_pcon_write_pipe;
-- 
2.31.1

Re: [PATCH 0/2] Fix race issues.

[Corinna Vinschen]

On Apr 19 19:30, Takashi Yano wrote:
> Takashi Yano (2):
>   Cygwin: console: Fix race issue regarding cons_master_thread().
>   Cygwin: pty: Fix race issue in inheritance of pseudo console.
> 
>  winsup/cygwin/fhandler_console.cc |  10 ++-
>  winsup/cygwin/fhandler_tty.cc     | 108 ++++++++++++++++++------------
>  winsup/cygwin/tty.cc              |  15 ++---
>  winsup/cygwin/tty.h               |   2 +-
>  4 files changed, 77 insertions(+), 58 deletions(-)
> 
> -- 
> 2.31.1

Pushed.


Thanks,
Corinna

RE: [PATCH 0/2] Fix race issues.

[David Allsopp]

Corinna Vinschen wrote:
> On Apr 19 19:30, Takashi Yano wrote:
> > Takashi Yano (2):
> >   Cygwin: console: Fix race issue regarding cons_master_thread().
> >   Cygwin: pty: Fix race issue in inheritance of pseudo console.
> >
> >  winsup/cygwin/fhandler_console.cc |  10 ++-
> >  winsup/cygwin/fhandler_tty.cc     | 108 ++++++++++++++++++------------
> >  winsup/cygwin/tty.cc              |  15 ++---
> >  winsup/cygwin/tty.h               |   2 +-
> >  4 files changed, 77 insertions(+), 58 deletions(-)
> >
> > --
> > 2.31.1
> 
> Pushed.

Armed with this morning's Cygwin snapshot, OCaml builds again as well. Many thanks!


David