From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id 8C6613972431; Fri, 8 May 2020 11:30:12 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8C6613972431 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: designate one reader as owner X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: 16e7c1057854728ca7768813c995e6d83f90db67 X-Git-Newrev: 606baf5566d683ac68e3414c457d124c7c7bdcc5 Message-Id: <20200508113012.8C6613972431@sourceware.org> Date: Fri, 8 May 2020 11:30:12 +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:12 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=606baf5566d683ac68e3414c457d124c7c7bdcc5 commit 606baf5566d683ac68e3414c457d124c7c7bdcc5 Author: Ken Brown Date: Thu Mar 26 14:32:10 2020 -0400 Cygwin: FIFO: designate one reader as owner Among all the open readers of a FIFO, one is declared to be the owner. This is the only reader that listens for client connections, and it is the only one that has an accurate fc_handler list. Add shared data and methods for getting and setting the owner, as well as a lock to prevent more than one reader from accessing these data simultaneously. Modify the fifo_reader_thread so that it checks the owner at the beginning of its loop. If there is no owner, it takes ownership. If there is an owner but it is a different reader, the thread just waits to be canceled. Otherwise, it listens for client connections as before. Remove the 'first' argument from create_pipe_instance. It is not needed, and it may be confusing in the future since only the owner knows whether a pipe instance is the first. When opening a reader, don't return until the fifo_reader_thread has time to set an owner. If the owner closes, indicate that there is no longer an owner. Clear the child's fc_handler list in dup, and don't bother duplicating the handles. The child never starts out as owner, so it can't use those handles. Do the same thing in fixup_after_fork in the close-on-exec case. In the non-close-on-exec case, the child inherits an fc_handler list that it can't use, but we can just leave it alone; the handles will be closed when the child is closed. Diff: --- winsup/cygwin/fhandler.h | 13 ++- winsup/cygwin/fhandler_fifo.cc | 237 ++++++++++++++++++++++------------------- 2 files changed, 141 insertions(+), 109 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 65aab4da3..bd44da5cd 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1324,10 +1324,17 @@ struct fifo_reader_id_t class fifo_shmem_t { LONG _nreaders; + fifo_reader_id_t _owner; + af_unix_spinlock_t _owner_lock; public: int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); } int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); } + + fifo_reader_id_t get_owner () const { return _owner; } + void set_owner (fifo_reader_id_t fr_id) { _owner = fr_id; } + void owner_lock () { _owner_lock.lock (); } + void owner_unlock () { _owner_lock.unlock (); } }; class fhandler_fifo: public fhandler_base @@ -1356,7 +1363,7 @@ class fhandler_fifo: public fhandler_base bool __reg2 wait (HANDLE); static NTSTATUS npfs_handle (HANDLE &); - HANDLE create_pipe_instance (bool); + HANDLE create_pipe_instance (); NTSTATUS open_pipe (HANDLE&); NTSTATUS wait_open_pipe (HANDLE&); int add_client_handler (); @@ -1384,6 +1391,10 @@ public: void fifo_client_unlock () { _fifo_client_lock.unlock (); } fifo_reader_id_t get_me () const { return me; } + fifo_reader_id_t get_owner () const { return shmem->get_owner (); } + void set_owner (fifo_reader_id_t fr_id) { shmem->set_owner (fr_id); } + void owner_lock () { shmem->owner_lock (); } + void owner_unlock () { shmem->owner_unlock (); } int open (int, mode_t); off_t lseek (off_t offset, int whence); diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 5676a2c97..0b9b33785 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -164,7 +164,7 @@ fhandler_fifo::npfs_handle (HANDLE &nph) blocking mode so that we can easily wait for a connection. After it is connected, it is put in nonblocking mode. */ HANDLE -fhandler_fifo::create_pipe_instance (bool first) +fhandler_fifo::create_pipe_instance () { NTSTATUS status; HANDLE npfsh; @@ -187,14 +187,12 @@ fhandler_fifo::create_pipe_instance (bool first) access = GENERIC_READ | FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES | SYNCHRONIZE; sharing = FILE_SHARE_READ | FILE_SHARE_WRITE; - hattr = openflags & O_CLOEXEC ? 0 : OBJ_INHERIT; - if (first) - hattr |= OBJ_CASE_INSENSITIVE; + hattr = (openflags & O_CLOEXEC ? 0 : OBJ_INHERIT) | OBJ_CASE_INSENSITIVE; InitializeObjectAttributes (&attr, get_pipe_name (), hattr, npfsh, NULL); timeout.QuadPart = -500000; status = NtCreateNamedPipeFile (&ph, access, &attr, &io, sharing, - first ? FILE_CREATE : FILE_OPEN, 0, + FILE_OPEN_IF, 0, FILE_PIPE_MESSAGE_TYPE | FILE_PIPE_REJECT_REMOTE_CLIENTS, FILE_PIPE_MESSAGE_MODE, @@ -292,14 +290,13 @@ fhandler_fifo::add_client_handler () int ret = -1; fifo_client_handler fc; HANDLE ph = NULL; - bool first = (nhandlers == 0); if (nhandlers == MAX_CLIENTS) { set_errno (EMFILE); goto out; } - ph = create_pipe_instance (first); + ph = create_pipe_instance (); if (!ph) goto out; else @@ -349,92 +346,120 @@ fhandler_fifo::fifo_reader_thread_func () while (1) { - /* Cleanup the fc_handler list. */ - fifo_client_lock (); - int i = 0; - while (i < nhandlers) + fifo_reader_id_t cur_owner; + + owner_lock (); + cur_owner = get_owner (); + if (!cur_owner) { - if (fc_handler[i].state < fc_connected) - delete_client_handler (i); - else - i++; + set_owner (me); + owner_unlock (); + continue; + } + else if (cur_owner != me) + { + owner_unlock (); + WaitForSingleObject (cancel_evt, INFINITE); + goto canceled; } + else + { + /* I'm the owner */ + fifo_client_lock (); - /* Create a new client handler. */ - if (add_client_handler () < 0) - api_fatal ("Can't add a client handler, %E"); + /* Cleanup the fc_handler list. */ + fifo_client_lock (); + int i = 0; + while (i < nhandlers) + { + if (fc_handler[i].state < fc_connected) + delete_client_handler (i); + else + i++; + } - /* Listen for a writer to connect to the new client handler. */ - fifo_client_handler& fc = fc_handler[nhandlers - 1]; - fifo_client_unlock (); - NTSTATUS status; - IO_STATUS_BLOCK io; - bool cancel = false; + /* Create a new client handler. */ + if (add_client_handler () < 0) + api_fatal ("Can't add a client handler, %E"); - status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, &io, - FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0); - if (status == STATUS_PENDING) - { - HANDLE w[2] = { conn_evt, cancel_evt }; - switch (WaitForMultipleObjects (2, w, false, INFINITE)) + /* Listen for a writer to connect to the new client handler. */ + fifo_client_handler& fc = fc_handler[nhandlers - 1]; + fifo_client_unlock (); + owner_unlock (); + NTSTATUS status; + IO_STATUS_BLOCK io; + bool cancel = false; + + status = NtFsControlFile (fc.h, conn_evt, NULL, NULL, &io, + FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0); + if (status == STATUS_PENDING) { - case WAIT_OBJECT_0: - status = io.Status; + HANDLE w[2] = { conn_evt, cancel_evt }; + switch (WaitForMultipleObjects (2, w, false, INFINITE)) + { + case WAIT_OBJECT_0: + status = io.Status; + debug_printf ("NtFsControlFile STATUS_PENDING, then %y", + status); + break; + case WAIT_OBJECT_0 + 1: + status = STATUS_THREAD_IS_TERMINATING; + cancel = true; + break; + default: + api_fatal ("WFMO failed, %E"); + } + } + else + debug_printf ("NtFsControlFile status %y, no STATUS_PENDING", + status); + HANDLE ph = NULL; + NTSTATUS status1; + + fifo_client_lock (); + switch (status) + { + case STATUS_SUCCESS: + case STATUS_PIPE_CONNECTED: + record_connection (fc); break; - case WAIT_OBJECT_0 + 1: - status = STATUS_THREAD_IS_TERMINATING; - cancel = true; + case STATUS_PIPE_CLOSING: + record_connection (fc, fc_closing); + break; + case STATUS_THREAD_IS_TERMINATING: + /* Try to connect a bogus client. Otherwise fc is still + listening, and the next connection might not get recorded. */ + status1 = open_pipe (ph); + WaitForSingleObject (conn_evt, INFINITE); + if (NT_SUCCESS (status1)) + /* Bogus cilent connected. */ + delete_client_handler (nhandlers - 1); + else + /* Did a real client connect? */ + switch (io.Status) + { + case STATUS_SUCCESS: + case STATUS_PIPE_CONNECTED: + record_connection (fc); + break; + case STATUS_PIPE_CLOSING: + record_connection (fc, fc_closing); + break; + default: + debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status); + fc.state = fc_unknown; + break; + } break; default: - api_fatal ("WFMO failed, %E"); + break; } + fifo_client_unlock (); + if (ph) + NtClose (ph); + if (cancel) + goto canceled; } - HANDLE ph = NULL; - NTSTATUS status1; - - fifo_client_lock (); - switch (status) - { - case STATUS_SUCCESS: - case STATUS_PIPE_CONNECTED: - record_connection (fc); - break; - case STATUS_PIPE_CLOSING: - record_connection (fc, fc_closing); - break; - case STATUS_THREAD_IS_TERMINATING: - /* Try to connect a bogus client. Otherwise fc is still - listening, and the next connection might not get recorded. */ - status1 = open_pipe (ph); - WaitForSingleObject (conn_evt, INFINITE); - if (NT_SUCCESS (status1)) - /* Bogus cilent connected. */ - delete_client_handler (nhandlers - 1); - else - /* Did a real client connect? */ - switch (io.Status) - { - case STATUS_SUCCESS: - case STATUS_PIPE_CONNECTED: - record_connection (fc); - break; - case STATUS_PIPE_CLOSING: - record_connection (fc, fc_closing); - break; - default: - debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status); - fc.state = fc_unknown; - break; - } - break; - default: - break; - } - fifo_client_unlock (); - if (ph) - NtClose (ph); - if (cancel) - goto canceled; } canceled: if (conn_evt) @@ -580,6 +605,15 @@ fhandler_fifo::open (int flags, mode_t) me.winpid = GetCurrentProcessId (); me.fh = this; new cygthread (fifo_reader_thread, this, "fifo_reader", thr_sync_evt); + /* Wait until there's an owner. */ + owner_lock (); + while (!get_owner ()) + { + owner_unlock (); + yield (); + owner_lock (); + } + owner_unlock (); /* If we're a duplexer, we need a handle for writing. */ if (duplexer) @@ -1014,6 +1048,10 @@ fhandler_fifo::close () if (dec_nreaders () == 0) ResetEvent (read_ready); cancel_reader_thread (); + owner_lock (); + if (get_owner () == me) + set_owner (null_fr_id); + owner_unlock (); if (cancel_evt) NtClose (cancel_evt); if (thr_sync_evt) @@ -1056,7 +1094,6 @@ fhandler_fifo::fcntl (int cmd, intptr_t arg) int fhandler_fifo::dup (fhandler_base *child, int flags) { - int i = 0; fhandler_fifo *fhf = NULL; if (get_flags () & O_PATH) @@ -1092,6 +1129,9 @@ fhandler_fifo::dup (fhandler_base *child, int flags) /* Make sure the child starts unlocked. */ fhf->fifo_client_unlock (); + /* Clear fc_handler list; the child never starts as owner. */ + fhf->nhandlers = 0; + if (!DuplicateHandle (GetCurrentProcess (), shmem_handle, GetCurrentProcess (), &fhf->shmem_handle, 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS)) @@ -1101,25 +1141,8 @@ fhandler_fifo::dup (fhandler_base *child, int flags) } if (fhf->reopen_shmem () < 0) goto err_close_shmem_handle; - fifo_client_lock (); - for (i = 0; i < nhandlers; i++) - { - if (!DuplicateHandle (GetCurrentProcess (), fc_handler[i].h, - GetCurrentProcess (), &fhf->fc_handler[i].h, - 0, !(flags & O_CLOEXEC), DUPLICATE_SAME_ACCESS)) - { - __seterrno (); - break; - } - } - if (i < nhandlers) - { - fifo_client_unlock (); - goto err_close_handlers; - } - fifo_client_unlock (); if (!(fhf->cancel_evt = create_event ())) - goto err_close_handlers; + goto err_close_shmem; if (!(fhf->thr_sync_evt = create_event ())) goto err_close_cancel_evt; inc_nreaders (); @@ -1129,9 +1152,7 @@ fhandler_fifo::dup (fhandler_base *child, int flags) return 0; err_close_cancel_evt: NtClose (fhf->cancel_evt); -err_close_handlers: - for (int j = 0; j < i; j++) - fhf->fc_handler[j].close (); +err_close_shmem: NtUnmapViewOfSection (GetCurrentProcess (), fhf->shmem); err_close_shmem_handle: NtClose (fhf->shmem_handle); @@ -1160,10 +1181,10 @@ fhandler_fifo::fixup_after_fork (HANDLE parent) fork_fixup (parent, shmem_handle, "shmem_handle"); if (reopen_shmem () < 0) api_fatal ("Can't reopen shared memory during fork, %E"); - fifo_client_lock (); - for (int i = 0; i < nhandlers; i++) - fork_fixup (parent, fc_handler[i].h, "fc_handler[].h"); - fifo_client_unlock (); + if (close_on_exec ()) + /* Prevent a later attempt to close the non-inherited + pipe-instance handles copied from the parent. */ + nhandlers = 0; if (!(cancel_evt = create_event ())) api_fatal ("Can't create reader thread cancel event during fork, %E"); if (!(thr_sync_evt = create_event ()))