[newlib-cygwin] Cygwin: FIFO: take ownership on exec

[newlib-cygwin] Cygwin: FIFO: take ownership on exec

[Ken Brown]

https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=39a9cd94651d306117c47ea1ac3eab45f6098d0e

commit 39a9cd94651d306117c47ea1ac3eab45f6098d0e
Author: Ken Brown <kbrown@cornell.edu>
Date:   Thu Apr 23 21:29:32 2020 -0400

    Cygwin: FIFO: take ownership on exec
    
    If fixup_after_exec is called on a non-close-on-exec reader whose
    parent is the owner, transfer ownership to the child.  Otherwise the
    parent's pipe handles will be closed before any other reader can
    duplicate them.
    
    To help with this, make the cancel_evt and thr_sync_evt handles
    inheritable, so that the child can terminate the parent's
    fifo_reader_thread (and the parent will update the shared fc_handler
    list).
    
    Add an optional argument 'from_exec' to update_my_handlers to simplify
    its use in this case; no handle duplication is required.

Diff:
---
 winsup/cygwin/fhandler.h       |   2 +-
 winsup/cygwin/fhandler_fifo.cc | 151 +++++++++++++++++++++++++++++------------
 2 files changed, 107 insertions(+), 46 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 34b209f5d..1cd7d2b11 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1414,7 +1414,7 @@ class fhandler_fifo: public fhandler_base
   { return shmem->get_shared_fc_handler_committed (); }
   void set_shared_fc_handler_committed (size_t n)
   { shmem->set_shared_fc_handler_committed (n); }
-  int update_my_handlers ();
+  int update_my_handlers (bool from_exec = false);
   int update_shared_handlers ();
 
 public:
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index fd5d9f805..5ae7b1f9c 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/winsup/cygwin/fhandler_fifo.cc
@@ -104,13 +104,14 @@ sec_user_cloexec (bool cloexec, PSECURITY_ATTRIBUTES sa, PSID sid)
 }
 
 static HANDLE
-create_event ()
+create_event (bool inherit = false)
 {
   NTSTATUS status;
   OBJECT_ATTRIBUTES attr;
   HANDLE evt = NULL;
 
-  InitializeObjectAttributes (&attr, NULL, 0, NULL, NULL);
+  InitializeObjectAttributes (&attr, NULL, inherit ? OBJ_INHERIT : 0,
+			      NULL, NULL);
   status = NtCreateEvent (&evt, EVENT_ALL_ACCESS, &attr,
 			  NotificationEvent, FALSE);
   if (!NT_SUCCESS (status))
@@ -353,47 +354,72 @@ fhandler_fifo::record_connection (fifo_client_handler& fc,
   set_pipe_non_blocking (fc.h, true);
 }
 
-/* Called from fifo_reader_thread_func with owner_lock in place. */
+/* Called from fifo_reader_thread_func with owner_lock in place, also
+   from fixup_after_exec with shared handles useable as they are. */
 int
-fhandler_fifo::update_my_handlers ()
+fhandler_fifo::update_my_handlers (bool from_exec)
 {
-  close_all_handlers ();
-  fifo_reader_id_t prev = get_prev_owner ();
-  if (!prev)
+  if (from_exec)
     {
-      debug_printf ("No previous owner to copy handles from");
-      return 0;
+      nhandlers = get_shared_nhandlers ();
+      if (nhandlers > shandlers)
+	{
+	  int save = shandlers;
+	  shandlers = nhandlers + 64;
+	  void *temp = realloc (fc_handler,
+				shandlers * sizeof (fc_handler[0]));
+	  if (!temp)
+	    {
+	      shandlers = save;
+	      nhandlers = 0;
+	      set_errno (ENOMEM);
+	      return -1;
+	    }
+	  fc_handler = (fifo_client_handler *) temp;
+	}
+      memcpy (fc_handler, shared_fc_handler,
+	      nhandlers * sizeof (fc_handler[0]));
     }
-  HANDLE prev_proc;
-  if (prev.winpid == me.winpid)
-    prev_proc =  GetCurrentProcess ();
   else
-    prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
-  if (!prev_proc)
     {
-      debug_printf ("Can't open process of previous owner, %E");
-      __seterrno ();
-      return -1;
-    }
-
-  for (int i = 0; i < get_shared_nhandlers (); i++)
-    {
-      /* Should never happen. */
-      if (shared_fc_handler[i].state < fc_connected)
-	continue;
-      if (add_client_handler (false) < 0)
-	api_fatal ("Can't add client handler, %E");
-      fifo_client_handler &fc = fc_handler[nhandlers - 1];
-      if (!DuplicateHandle (prev_proc, shared_fc_handler[i].h,
-			    GetCurrentProcess (), &fc.h, 0,
-			    !close_on_exec (), DUPLICATE_SAME_ACCESS))
+      close_all_handlers ();
+      fifo_reader_id_t prev = get_prev_owner ();
+      if (!prev)
+	{
+	  debug_printf ("No previous owner to copy handles from");
+	  return 0;
+	}
+      HANDLE prev_proc;
+      if (prev.winpid == me.winpid)
+	prev_proc =  GetCurrentProcess ();
+      else
+	prev_proc = OpenProcess (PROCESS_DUP_HANDLE, false, prev.winpid);
+      if (!prev_proc)
 	{
-	  debug_printf ("Can't duplicate handle of previous owner, %E");
-	  --nhandlers;
+	  debug_printf ("Can't open process of previous owner, %E");
 	  __seterrno ();
 	  return -1;
 	}
-      fc.state = shared_fc_handler[i].state;
+
+      for (int i = 0; i < get_shared_nhandlers (); i++)
+	{
+	  /* Should never happen. */
+	  if (shared_fc_handler[i].state < fc_connected)
+	    continue;
+	  if (add_client_handler (false) < 0)
+	    api_fatal ("Can't add client handler, %E");
+	  fifo_client_handler &fc = fc_handler[nhandlers - 1];
+	  if (!DuplicateHandle (prev_proc, shared_fc_handler[i].h,
+				GetCurrentProcess (), &fc.h, 0,
+				!close_on_exec (), DUPLICATE_SAME_ACCESS))
+	    {
+	      debug_printf ("Can't duplicate handle of previous owner, %E");
+	      --nhandlers;
+	      __seterrno ();
+	      return -1;
+	    }
+	  fc.state = shared_fc_handler[i].state;
+	}
     }
   return 0;
 }
@@ -771,9 +797,9 @@ fhandler_fifo::open (int flags, mode_t)
       if (create_shared_fc_handler () < 0)
 	goto err_close_shmem;
       inc_nreaders ();
-      if (!(cancel_evt = create_event ()))
+      if (!(cancel_evt = create_event (true)))
 	goto err_dec_nreaders;
-      if (!(thr_sync_evt = create_event ()))
+      if (!(thr_sync_evt = create_event (true)))
 	goto err_close_cancel_evt;
       me.winpid = GetCurrentProcessId ();
       me.fh = this;
@@ -1338,9 +1364,9 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
 	}
       if (fhf->reopen_shared_fc_handler () < 0)
 	goto err_close_shared_fc_hdl;
-      if (!(fhf->cancel_evt = create_event ()))
+      if (!(fhf->cancel_evt = create_event (true)))
 	goto err_close_shared_fc_handler;
-      if (!(fhf->thr_sync_evt = create_event ()))
+      if (!(fhf->thr_sync_evt = create_event (true)))
 	goto err_close_cancel_evt;
       inc_nreaders ();
       fhf->me.fh = fhf;
@@ -1389,9 +1415,17 @@ fhandler_fifo::fixup_after_fork (HANDLE parent)
 	/* Prevent a later attempt to close the non-inherited
 	   pipe-instance handles copied from the parent. */
 	nhandlers = 0;
-      if (!(cancel_evt = create_event ()))
+      else
+	{
+	  /* Close inherited handles needed only by exec. */
+	  if (cancel_evt)
+	    NtClose (cancel_evt);
+	  if (thr_sync_evt)
+	    NtClose (thr_sync_evt);
+	}
+      if (!(cancel_evt = create_event (true)))
 	api_fatal ("Can't create reader thread cancel event during fork, %E");
-      if (!(thr_sync_evt = create_event ()))
+      if (!(thr_sync_evt = create_event (true)))
 	api_fatal ("Can't create reader thread sync event during fork, %E");
       inc_nreaders ();
       me.winpid = GetCurrentProcessId ();
@@ -1414,10 +1448,32 @@ fhandler_fifo::fixup_after_exec ()
 	api_fatal ("Can't reopen shared fc_handler memory during exec, %E");
       fc_handler = NULL;
       nhandlers = shandlers = 0;
+
+      /* Cancel parent's reader thread */
+      if (cancel_evt)
+	SetEvent (cancel_evt);
+      if (thr_sync_evt)
+	WaitForSingleObject (thr_sync_evt, INFINITE);
+
+      /* Take ownership if parent is owner. */
+      fifo_reader_id_t parent_fr = me;
       me.winpid = GetCurrentProcessId ();
-      if (!(cancel_evt = create_event ()))
+      owner_lock ();
+      if (get_owner () == parent_fr)
+	{
+	  set_owner (me);
+	  if (update_my_handlers (true) < 0)
+	    api_fatal ("Can't update my handlers, %E");
+	}
+      owner_unlock ();
+      /* Close inherited cancel_evt and thr_sync_evt. */
+      if (cancel_evt)
+	NtClose (cancel_evt);
+      if (thr_sync_evt)
+	NtClose (thr_sync_evt);
+      if (!(cancel_evt = create_event (true)))
 	api_fatal ("Can't create reader thread cancel event during exec, %E");
-      if (!(thr_sync_evt = create_event ()))
+      if (!(thr_sync_evt = create_event (true)))
 	api_fatal ("Can't create reader thread sync event during exec, %E");
       /* At this moment we're a new reader.  The count will be
 	 decremented when the parent closes. */
@@ -1433,8 +1489,13 @@ fhandler_fifo::set_close_on_exec (bool val)
   set_no_inheritance (read_ready, val);
   set_no_inheritance (write_ready, val);
   set_no_inheritance (writer_opening, val);
-  fifo_client_lock ();
-  for (int i = 0; i < nhandlers; i++)
-    set_no_inheritance (fc_handler[i].h, val);
-  fifo_client_unlock ();
+  if (reader)
+    {
+      set_no_inheritance (cancel_evt, val);
+      set_no_inheritance (thr_sync_evt, val);
+      fifo_client_lock ();
+      for (int i = 0; i < nhandlers; i++)
+	set_no_inheritance (fc_handler[i].h, val);
+      fifo_client_unlock ();
+    }
 }