From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id 82C6D386F471; Fri, 22 May 2020 14:37:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 82C6D386F471 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: Revert "take ownership on exec" X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: 0365031ce1347600d854a23f30f1355745a1765c X-Git-Newrev: fe937e21ad6041fad12751d3e4867602e2737739 Message-Id: <20200522143733.82C6D386F471@sourceware.org> Date: Fri, 22 May 2020 14:37:33 +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, 22 May 2020 14:37:33 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=fe937e21ad6041fad12751d3e4867602e2737739 commit fe937e21ad6041fad12751d3e4867602e2737739 Author: Ken Brown 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);