Re: cygwin-2 process handling

[Corinna Vinschen]

On May 27 17:17, Corinna Vinschen wrote:
> On May 27 14:23, Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On May 21 20:53, Takashi Yano wrote:
> > > [...]
> > > +  if (output_handle_local)
> > > +    {
> > > +      OBJECT_BASIC_INFORMATION obi;
> > > +      NTSTATUS status;
> > > +      ULONG hdl_cnt = 0;
> > > +
> > > +      status = NtQueryObject (output_handle_local, 
> > > ObjectBasicInformation,
> > > +   &obi, sizeof obi, NULL);
> > > +      if (!NT_SUCCESS (status))
> > > + debug_printf ("NtQueryObject: %y", status);
> > > +      else
> > > + hdl_cnt = obi.HandleCount;
> > > +      termios_printf("HandleCount: %d", hdl_cnt);
> > > +      if (hdl_cnt == 1)
> > > + SetEvent (input_available_event);
> > > +      CloseHandle (output_handle_local);
> > > +    }
> >  
> > Isn't that racy?  Consider two processes doing that at the same time.
> > Both calls to NtQueryObject could come up with hdl_cnt == 2 and the
> > problem persists.
> > 
> > Wouldn't it be safer to call SetEvent(input_available_event) all the
> > time from here?
> 
> We discussed this already in March and only briefly talked about a
> change like this requiring changes to fhandler_pty_slave::read.
> However, I don't see this.  The read code already takes 0 bytes input
> and broken pipe scenarios into account.  Do you see something needing
> a change I don't?

Never mind, it doesn't work quite as expected, so changes would be
required.

I created another version of your patch which avoids duplicating the
tested handle and makes the test-and-close-handle operation atomic:

        * fhandler_tty.cc (fhandler_pty_common::close): Don't close output_mutex
        here.  Move into callers.
        (fhandler_pty_master::close): Use NtQueryObject instead of
        PeekNamedPipe to detect closing the last master handle.

diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
index e91b3e3..12f6124 100644
--- a/winsup/cygwin/fhandler_tty.cc
+++ b/winsup/cygwin/fhandler_tty.cc
@@ -552,7 +552,10 @@ fhandler_pty_slave::close ()
        get_output_handle_cyg ());
   if ((unsigned) myself->ctty == FHDEV (DEV_PTYS_MAJOR, get_minor ()))
     fhandler_console::free_console (); /* assumes that we are the last pty 
closer */
-  return fhandler_pty_common::close ();
+  fhandler_pty_common::close ();
+  if (!ForceCloseHandle (output_mutex))
+    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
+  return 0;
 }
 
 int
@@ -1259,8 +1262,6 @@ fhandler_pty_common::close ()
   termios_printf ("pty%d <%p,%p> closing", get_minor (), get_handle (), 
get_output_handle ());
   if (!ForceCloseHandle (input_mutex))
     termios_printf ("CloseHandle (input_mutex<%p>), %E", input_mutex);
-  if (!ForceCloseHandle (output_mutex))
-    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
   if (!ForceCloseHandle1 (get_handle (), from_pty))
     termios_printf ("CloseHandle (get_handle ()<%p>), %E", get_handle ());
   if (!ForceCloseHandle1 (get_output_handle (), to_pty))
@@ -1281,6 +1282,9 @@ fhandler_pty_master::cleanup ()
 int
 fhandler_pty_master::close ()
 {
+  OBJECT_BASIC_INFORMATION obi;
+  NTSTATUS status;
+
   termios_printf ("closing from_master(%p)/to_master(%p)/to_master_cyg(%p) 
since we own them(%u)",
                  from_master, to_master, to_master_cyg, dwProcessId);
   if (cygwin_finished_initializing)
@@ -1309,13 +1313,22 @@ fhandler_pty_master::close ()
        }
     }
 
-  fhandler_pty_common::close ();
-
   /* Check if the last master handle has been closed.  If so, set
      input_available_event to wake up potentially waiting slaves. */
-  if (!PeekNamedPipe (from_master, NULL, 0, NULL, NULL, NULL)
-      && GetLastError () == ERROR_BROKEN_PIPE) 
-    SetEvent (input_available_event);
+  acquire_output_mutex (INFINITE);
+  status = NtQueryObject (get_output_handle (), ObjectBasicInformation,
+                         &obi, sizeof obi, NULL);
+  fhandler_pty_common::close ();
+  release_output_mutex ();
+  if (!ForceCloseHandle (output_mutex))
+    termios_printf ("CloseHandle (output_mutex<%p>), %E", output_mutex);
+  if (!NT_SUCCESS (status))
+    debug_printf ("NtQueryObject: %y", status);
+  else if (obi.HandleCount == 1)
+    {
+      termios_printf("Closing last master");
+      SetEvent (input_available_event);
+    }
 
   if (!ForceCloseHandle (from_master))
     termios_printf ("error closing from_master %p, %E", from_master);

Does that look ok?  It fixes the reported problem for me.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

pgp2AtKGtAz0B.pgp
Description: PGP signature