Cygwin: console: Fix a race issue between close() and open().

The open() call for console sometimes fails if the console owner
process is closing the console by close() at the same time. This
is due to mismatch state of con.owner variable and attaching state
to the console. With this patch, checking con.owner and attaching
to con.owner sequence in open(), and resetting con.owner and freeing
console sequence in close() are guarded by output_mutex to avoid
such a race issue.
Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255575.html

Fixes: 3721a756b0d8 ("Cygwin: console: Make the console accessible from other terminals.")
Reported-by: Kate Deplaix <kit-ty-kate@outlook.com>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>

diff --git a/winsup/cygwin/fhandler/console.cc b/winsup/cygwin/fhandler/console.cc
index 67ea95466338f0d43d1871ee2325fb3dcf183aae..1c0d5c815c498049f153da05fe0dc2abcc328e63 100644 (file)
--- a/winsup/cygwin/fhandler/console.cc
+++ b/winsup/cygwin/fhandler/console.cc
@@ -687,14 +687,6 @@ fhandler_console::set_unit ()
     {
       devset = (fh_devices) shared_console_info[unit]->tty_min_state.getntty ();
       _tc = &(shared_console_info[unit]->tty_min_state);
-      if (!created)
-       {
-         while (con.owner > MAX_PID)
-           Sleep (1);
-         pinfo p (con.owner);
-         if (!p)
-           con.owner = myself->pid;
-       }
     }
 
   dev ().parse (devset);
@@ -1744,11 +1736,23 @@ fhandler_console::open (int flags, mode_t)
   set_handle (NULL);
   set_output_handle (NULL);
 
+  setup_io_mutex ();
+  acquire_output_mutex (mutex_timeout);
+
+  do
+    {
+      pinfo p (con.owner);
+      if (!p)
+       con.owner = myself->pid;
+    }
+  while (false);
+
   /* Open the input handle as handle_ */
   bool err = false;
   DWORD resume_pid = attach_console (con.owner, &err);
   if (err)
     {
+      release_output_mutex ();
       set_errno (EACCES);
       return 0;
     }
@@ -1759,6 +1763,7 @@ fhandler_console::open (int flags, mode_t)
 
   if (h == INVALID_HANDLE_VALUE)
     {
+      release_output_mutex ();
       __seterrno ();
       return 0;
     }
@@ -1768,6 +1773,7 @@ fhandler_console::open (int flags, mode_t)
   resume_pid = attach_console (con.owner, &err);
   if (err)
     {
+      release_output_mutex ();
       set_errno (EACCES);
       return 0;
     }
@@ -1778,14 +1784,16 @@ fhandler_console::open (int flags, mode_t)
 
   if (h == INVALID_HANDLE_VALUE)
     {
+      release_output_mutex ();
       __seterrno ();
       return 0;
     }
   set_output_handle (h);
   handle_set.output_handle = h;
+  release_output_mutex ();
+
   wpbuf.init ();
 
-  setup_io_mutex ();
   handle_set.input_mutex = input_mutex;
   handle_set.output_mutex = output_mutex;
 
@@ -1895,25 +1903,20 @@ fhandler_console::close ()
        }
     }
 
-  release_output_mutex ();
-
-  if (shared_console_info[unit] && con.owner == myself->pid
-      && master_thread_started)
+  if (shared_console_info[unit] && con.owner == myself->pid)
     {
-      char name[MAX_PATH];
-      shared_name (name, CONS_THREAD_SYNC, get_minor ());
-      thread_sync_event = OpenEvent (MAXIMUM_ALLOWED, FALSE, name);
-      con.owner = MAX_PID + 1;
-      WaitForSingleObject (thread_sync_event, INFINITE);
-      CloseHandle (thread_sync_event);
+      if (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 = MAX_PID + 1;
+         WaitForSingleObject (thread_sync_event, INFINITE);
+         CloseHandle (thread_sync_event);
+       }
       con.owner = 0;
     }
 
-  CloseHandle (input_mutex);
-  input_mutex = NULL;
-  CloseHandle (output_mutex);
-  output_mutex = NULL;
-
   CloseHandle (get_handle ());
   CloseHandle (get_output_handle ());
 
@@ -1926,6 +1929,13 @@ fhandler_console::close ()
          || get_device () == (dev_t) myself->ctty))
     free_console ();
 
+  release_output_mutex ();
+
+  CloseHandle (input_mutex);
+  input_mutex = NULL;
+  CloseHandle (output_mutex);
+  output_mutex = NULL;
+
   if (shared_console_info[unit] && myself->ctty != tc ()->ntty)
     {
       UnmapViewOfFile ((void *) shared_console_info[unit]);

diff --git a/winsup/cygwin/release/3.5.2 b/winsup/cygwin/release/3.5.2
index 7d8df9489d8145c2e82dc66c7fec797d776fdcd1..fd3c768ded253b8ee9a27994763a0b47fdc68731 100644 (file)
--- a/winsup/cygwin/release/3.5.2
+++ b/winsup/cygwin/release/3.5.2
@@ -5,3 +5,7 @@ Fixes:
   is already unmapped due to race condition. To avoid this issue,
   shared console memory will be kept mapped if it belongs to CTTY.
   Addresses:  https://cygwin.com/pipermail/cygwin/2024-February/255561.html
+
+- Fix a race issue between console open() and close() which is caused
+  by state mismatch between con.owner and console attaching state.
+  Addresses: https://cygwin.com/pipermail/cygwin/2024-March/255575.html