[newlib-cygwin] Cygwin: FIFO: Revert "take ownership on exec"
Ken Brown
kbrown@sourceware.org
Fri May 22 14:37:33 GMT 2020
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=fe937e21ad6041fad12751d3e4867602e2737739
commit fe937e21ad6041fad12751d3e4867602e2737739
Author: Ken Brown <kbrown@cornell.edu>
Date: Tue May 19 10:14:10 2020 -0400
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
Diff:
---
winsup/cygwin/fhandler.h | 2 +-
winsup/cygwin/fhandler_fifo.cc | 137 ++++++++++++-----------------------------
2 files changed, 40 insertions(+), 99 deletions(-)
diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index 857f0a4e0..76ad2aab0 100644
--- a/winsup/cygwin/fhandler.h
+++ b/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 (); }
diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc
index e8a05dfbf..ab4b93942 100644
--- a/winsup/cygwin/fhandler_fifo.cc
+++ b/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,
- NULL, NULL);
+ InitializeObjectAttributes (&attr, NULL, 0, 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
- from fixup_after_exec with shared handles useable as they are. */
+/* Called from fifo_reader_thread_func with owner_lock in place. */
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 ();
- 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]));
+ 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)
{
- 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 open process of previous owner, %E");
+ __seterrno ();
+ 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 ("Can't open process of previous owner, %E");
+ debug_printf ("Can't duplicate handle of previous owner, %E");
+ --nhandlers;
__seterrno ();
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 ("Can't duplicate handle of previous owner, %E");
- --nhandlers;
- __seterrno ();
- return -1;
- }
- fc.state = shared_fc_handler[i].state;
- }
+ 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 (true)))
+ if (!(cancel_evt = create_event ()))
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
- {
- /* 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)))
+ if (!(cancel_evt = create_event ()))
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;
-
- /* 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)))
+ if (!(cancel_evt = create_event ()))
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);
More information about the Cygwin-cvs
mailing list