Cygwin: FIFO: Revert "take ownership on exec"

This reverts commit 39a9cd94651d306117c47ea1ac3eab45f6098d0e.

There is no need to explicitly take ownership in fixup_after_exec; if
ownership transfer is needed, it will be taken care of by
fhandler_fifo::close when the parent closes.  Moreover, closing the
parent's fifo_reader_thread can cause problems, such as the one
reported here:

  https://cygwin.com/pipermail/cygwin-patches/2020q2/010235.html

winsup/cygwin/fhandler.h

@@ -1443,7 +1443,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 (bool from_exec = false);
+  int update_my_handlers ();
   int update_shared_handlers ();
   bool shared_fc_handler_updated () const
   { return shmem->shared_fc_handler_updated (); }

winsup/cygwin/fhandler_fifo.cc

@@ -118,14 +118,13 @@ sec_user_cloexec (bool cloexec, PSECURITY_ATTRIBUTES sa, PSID sid)
 }
 
 static HANDLE
-create_event (bool inherit = false)
+create_event ()
 {
   NTSTATUS status;
   OBJECT_ATTRIBUTES attr;
   HANDLE evt = NULL;
 
-  InitializeObjectAttributes (&attr, NULL, inherit ? OBJ_INHERIT : 0,
+  InitializeObjectAttributes (&attr, NULL, 0, NULL, NULL);
-			      NULL, NULL);
   status = NtCreateEvent (&evt, EVENT_ALL_ACCESS, &attr,
 			  NotificationEvent, FALSE);
   if (!NT_SUCCESS (status))
@@ -368,69 +367,44 @@ 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, also
+/* Called from fifo_reader_thread_func with owner_lock in place. */
-   from fixup_after_exec with shared handles useable as they are. */
 int
-fhandler_fifo::update_my_handlers (bool from_exec)
+fhandler_fifo::update_my_handlers ()
 {
-  if (from_exec)
+  close_all_handlers ();
+  fifo_reader_id_t prev = get_prev_owner ();
+  if (!prev)
     {
-      nhandlers = get_shared_nhandlers ();
+      debug_printf ("No previous owner to copy handles from");
-      if (nhandlers > shandlers)
+      return 0;
-	{
-	  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)
     {
-      close_all_handlers ();
+      debug_printf ("Can't open process of previous owner, %E");
-      fifo_reader_id_t prev = get_prev_owner ();
+      __seterrno ();
-      if (!prev)
+      return -1;
+    }
+
+  for (int i = 0; i < get_shared_nhandlers (); i++)
+    {
+      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 ("No previous owner to copy handles from");
+	  debug_printf ("Can't duplicate handle of previous owner, %E");
-	  return 0;
+	  --nhandlers;
-	}
-      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;
 	}
-
+      fc.state = shared_fc_handler[i].state;
-      for (int i = 0; i < get_shared_nhandlers (); i++)
-	{
-	  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;
 }
@@ -918,10 +892,9 @@ fhandler_fifo::open (int flags, mode_t)
 	  __seterrno ();
 	  goto err_close_check_write_ready_evt;
 	}
-      /* Make cancel and sync inheritable for exec. */
+      if (!(cancel_evt = create_event ()))
-      if (!(cancel_evt = create_event (true)))
 	goto err_close_write_ready_ok_evt;
-      if (!(thr_sync_evt = create_event (true)))
+      if (!(thr_sync_evt = create_event ()))
 	goto err_close_cancel_evt;
       me.winpid = GetCurrentProcessId ();
       me.fh = this;
@@ -1626,9 +1599,9 @@ fhandler_fifo::dup (fhandler_base *child, int flags)
 	  __seterrno ();
 	  goto err_close_check_write_ready_evt;
 	}
-      if (!(fhf->cancel_evt = create_event (true)))
+      if (!(fhf->cancel_evt = create_event ()))
 	goto err_close_write_ready_ok_evt;
-      if (!(fhf->thr_sync_evt = create_event (true)))
+      if (!(fhf->thr_sync_evt = create_event ()))
 	goto err_close_cancel_evt;
       inc_nreaders ();
       fhf->me.fh = fhf;
@@ -1692,17 +1665,9 @@ 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;
-      else
+      if (!(cancel_evt = create_event ()))
-	{
-	  /* 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 (true)))
+      if (!(thr_sync_evt = create_event ()))
 	api_fatal ("Can't create reader thread sync event during fork, %E");
       inc_nreaders ();
       me.winpid = GetCurrentProcessId ();
@@ -1725,36 +1690,14 @@ fhandler_fifo::fixup_after_exec ()
 	api_fatal ("Can't reopen shared fc_handler memory during exec, %E");
       fc_handler = NULL;
       nhandlers = shandlers = 0;
-
+      if (!(cancel_evt = create_event ()))
-      /* 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 ();
-      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 (true)))
+      if (!(thr_sync_evt = create_event ()))
 	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. */
       inc_nreaders ();
+      me.winpid = GetCurrentProcessId ();
       new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt);
     }
 }
@@ -1773,8 +1716,6 @@ fhandler_fifo::set_close_on_exec (bool val)
       set_no_inheritance (update_needed_evt, val);
       set_no_inheritance (check_write_ready_evt, val);
       set_no_inheritance (write_ready_ok_evt, val);
-      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);