public inbox for cygwin-cvs@sourceware.org help / color / mirror / Atom feed
From: Ken Brown <kbrown@sourceware.org> To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: FIFO: take ownership on exec Date: Fri, 8 May 2020 11:30:27 +0000 (GMT) [thread overview] Message-ID: <20200508113027.A97D2397249F@sourceware.org> (raw) 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 (); + } }
reply other threads:[~2020-05-08 11:30 UTC|newest] Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200508113027.A97D2397249F@sourceware.org \ --to=kbrown@sourceware.org \ --cc=cygwin-cvs@sourceware.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).