From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id A97D2397249F; Fri, 8 May 2020 11:30:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A97D2397249F Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Ken Brown To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: FIFO: take ownership on exec X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: d9918451e2bea933955235e07290baf7993e8813 X-Git-Newrev: 39a9cd94651d306117c47ea1ac3eab45f6098d0e Message-Id: <20200508113027.A97D2397249F@sourceware.org> Date: Fri, 8 May 2020 11:30:27 +0000 (GMT) X-BeenThere: cygwin-cvs@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component git logs List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 May 2020 11:30:27 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=39a9cd94651d306117c47ea1ac3eab45f6098d0e commit 39a9cd94651d306117c47ea1ac3eab45f6098d0e Author: Ken Brown 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 (); + } }