Remove fifo_client_handler::connect and move its code into listen_client_thread. That way we can check the return status when a client handler's connect_evt is signaled. Previously we incorrectly assumed there was a successful connection.
Also simplify listen_client_thread in the following ways: - Replace fhandler_fifo::disconnect_and_reconnect by a new delete_client_handler method. Now we just delete invalid client handlers rather than trying to re-use them. - Try to maintain a client handler list that consists of connected client handlers and exactly one that is listening for a connection. This allows us to call WaitForMultipleObjects with only two wait objects. - Remove 'dummy_evt' from the fifo_client_handler struct; it is no longer needed. - On exit from listen_client_thread, delete the "extra" (listening) client handler. Otherwise there could be a connection that doesn't get recorded in the client handler list. This could happen when a file descriptor is being duplicated. --- winsup/cygwin/fhandler.h | 11 +- winsup/cygwin/fhandler_fifo.cc | 251 ++++++++++++++------------------- 2 files changed, 109 insertions(+), 153 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index fd205a6db..8fb176b24 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1250,13 +1250,10 @@ struct fifo_client_handler fhandler_base *fh; fifo_client_connect_state state; HANDLE connect_evt; - HANDLE dummy_evt; /* Never signaled. */ - fifo_client_handler () : fh (NULL), state (fc_unknown), connect_evt (NULL), - dummy_evt (NULL) {} + fifo_client_handler () : fh (NULL), state (fc_unknown), connect_evt (NULL) {} fifo_client_handler (fhandler_base *_fh, fifo_client_connect_state _state, - HANDLE _connect_evt, HANDLE _dummy_evt) - : fh (_fh), state (_state), connect_evt (_connect_evt), - dummy_evt (_dummy_evt) {} + HANDLE _connect_evt) + : fh (_fh), state (_state), connect_evt (_connect_evt) {} int connect (); int close (); }; @@ -1278,8 +1275,8 @@ class fhandler_fifo: public fhandler_base NTSTATUS npfs_handle (HANDLE &); HANDLE create_pipe_instance (bool); NTSTATUS open_pipe (); - int disconnect_and_reconnect (int); int add_client_handler (); + void delete_client_handler (int); bool listen_client (); int stop_listen_client (); public: diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index ce078d74d..abbb29a61 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -121,62 +121,6 @@ set_pipe_non_blocking (HANDLE ph, bool nonblocking) debug_printf ("NtSetInformationFile(FilePipeInformation): %y", status); } -/* The pipe instance is always in blocking mode when this is called. */ -int -fifo_client_handler::connect () -{ - NTSTATUS status; - IO_STATUS_BLOCK io; - - if (connect_evt) - ResetEvent (connect_evt); - else if (!(connect_evt = create_event ())) - return -1; - status = NtFsControlFile (fh->get_handle (), connect_evt, NULL, NULL, &io, - FSCTL_PIPE_LISTEN, NULL, 0, NULL, 0); - switch (status) - { - case STATUS_PENDING: - case STATUS_PIPE_LISTENING: - state = fc_connecting; - break; - case STATUS_PIPE_CONNECTED: - state = fc_connected; - set_pipe_non_blocking (fh->get_handle (), true); - break; - default: - __seterrno_from_nt_status (status); - return -1; - } - return 0; -} - -int -fhandler_fifo::disconnect_and_reconnect (int i) -{ - NTSTATUS status; - IO_STATUS_BLOCK io; - HANDLE ph = fc_handler[i].fh->get_handle (); - - status = NtFsControlFile (ph, NULL, NULL, NULL, &io, FSCTL_PIPE_DISCONNECT, - NULL, 0, NULL, 0); - /* Short-lived. Don't use cygwait. We don't want to be interrupted. */ - if (status == STATUS_PENDING - && NtWaitForSingleObject (ph, FALSE, NULL) == WAIT_OBJECT_0) - status = io.Status; - if (!NT_SUCCESS (status)) - { - __seterrno_from_nt_status (status); - return -1; - } - set_pipe_non_blocking (fc_handler[i].fh->get_handle (), false); - if (fc_handler[i].connect () < 0) - return -1; - if (fc_handler[i].state == fc_connected) - nconnected++; - return 0; -} - NTSTATUS fhandler_fifo::npfs_handle (HANDLE &nph) { @@ -283,41 +227,49 @@ fhandler_fifo::open_pipe () int fhandler_fifo::add_client_handler () { + int ret = -1; fifo_client_handler fc; fhandler_base *fh; + HANDLE ph = NULL; bool first = (nhandlers == 0); if (nhandlers == MAX_CLIENTS) { set_errno (EMFILE); - return -1; + goto out; } - if (!(fc.dummy_evt = create_event ())) - return -1; + if (!(fc.connect_evt = create_event ())) + goto out; if (!(fh = build_fh_dev (dev ()))) { set_errno (EMFILE); - return -1; + goto out; } - fc.fh = fh; - HANDLE ph = create_pipe_instance (first); + ph = create_pipe_instance (first); if (!ph) - goto errout; - fh->set_handle (ph); - fh->set_flags (get_flags ()); - if (fc.connect () < 0) { - fc.close (); - goto errout; + delete fh; + goto out; } - if (fc.state == fc_connected) - nconnected++; - fc_handler[nhandlers++] = fc; - return 0; -errout: - delete fh; - return -1; + else + { + fh->set_handle (ph); + fh->set_flags (get_flags ()); + ret = 0; + fc.fh = fh; + fc_handler[nhandlers++] = fc; + } +out: + return ret; +} +void +fhandler_fifo::delete_client_handler (int i) +{ + fc_handler[i].close (); + if (i < --nhandlers) + memmove (fc_handler + i, fc_handler + i + 1, + (nhandlers - i) * sizeof (fc_handler[i])); } /* Just hop to the listen_client_thread method. */ @@ -358,46 +310,23 @@ fhandler_fifo::listen_client_thread () while (1) { - bool found; - HANDLE w[MAX_CLIENTS + 1]; - int i; - DWORD wait_ret; + /* At the beginning of the loop, all client handlers are + in the fc_connected or fc_invalid state. */ + /* Delete any invalid clients. */ fifo_client_lock (); - found = false; - for (i = 0; i < nhandlers; i++) - switch (fc_handler[i].state) - { - case fc_invalid: - if (disconnect_and_reconnect (i) < 0) - { - fifo_client_unlock (); - goto out; - } - else - /* Recheck fc_handler[i].state. */ - i--; - break; - case fc_connected: - w[i] = fc_handler[i].dummy_evt; - break; - case fc_connecting: - found = true; - w[i] = fc_handler[i].connect_evt; - break; - case fc_unknown: /* Shouldn't happen. */ - default: - break; - } - w[nhandlers] = lct_termination_evt; - int res = 0; - if (!found) - res = add_client_handler (); - fifo_client_unlock (); - if (res < 0) + int i = 0; + while (i < nhandlers) + { + if (fc_handler[i].state == fc_invalid) + delete_client_handler (i); + else + i++; + } + + /* Create a new client handler. */ + if (add_client_handler () < 0) goto out; - else if (!found) - continue; /* Allow a writer to open. */ if (!arm (read_ready)) @@ -405,26 +334,73 @@ fhandler_fifo::listen_client_thread () __seterrno (); goto out; } + fifo_client_unlock (); - /* Wait for a client to connect. */ - wait_ret = WaitForMultipleObjects (nhandlers + 1, w, false, INFINITE); - i = wait_ret - WAIT_OBJECT_0; - if (i < 0 || i > nhandlers) - goto out; - else if (i == nhandlers) /* Thread termination requested. */ + /* Listen for a writer to connect to the new client handler. */ + fifo_client_handler& fc = fc_handler[nhandlers - 1]; + do + { + NTSTATUS status; + IO_STATUS_BLOCK io; + + status = NtFsControlFile (fc.fh->get_handle (), fc.connect_evt, + NULL, NULL, &io, FSCTL_PIPE_LISTEN, + NULL, 0, NULL, 0); + if (status == STATUS_PENDING) + { + HANDLE w[2] = { fc.connect_evt, lct_termination_evt }; + DWORD waitret = WaitForMultipleObjects (2, w, false, INFINITE); + switch (waitret) + { + case WAIT_OBJECT_0: + status = io.Status; + break; + case WAIT_OBJECT_0 + 1: + ret = 0; + status = STATUS_THREAD_IS_TERMINATING; + break; + default: + __seterrno (); + debug_printf ("WaitForMultipleObjects failed, %E"); + status = STATUS_THREAD_IS_TERMINATING; + break; + } + } + switch (status) + { + case STATUS_SUCCESS: + case STATUS_PIPE_CONNECTED: + fifo_client_lock (); + fc.state = fc_connected; + nconnected++; + set_pipe_non_blocking (fc.fh->get_handle (), true); + fifo_client_unlock (); + break; + case STATUS_PIPE_LISTENING: + /* Retry. */ + fc.state = fc_connecting; + ResetEvent (fc.connect_evt); + break; + case STATUS_THREAD_IS_TERMINATING: + fifo_client_lock (); + delete_client_handler (nhandlers - 1); + fifo_client_unlock (); + goto out; + default: + __seterrno_from_nt_status (status); + fifo_client_lock (); + delete_client_handler (nhandlers - 1); + fifo_client_unlock (); + goto out; + } + } while (fc.state == fc_connecting); + /* Check for thread termination in case WaitForMultipleObjects + didn't get called above. */ + if (IsEventSignalled (lct_termination_evt)) { ret = 0; goto out; } - else - { - fifo_client_lock (); - fc_handler[i].state = fc_connected; - nconnected++; - set_pipe_non_blocking (fc_handler[i].fh->get_handle (), true); - fifo_client_unlock (); - yield (); - } } out: ResetEvent (read_ready); @@ -490,7 +466,7 @@ fhandler_fifo::open (int flags, mode_t) /* If we're a duplexer, create the pipe and the first client handler. */ if (duplexer) { - HANDLE ph, connect_evt, dummy_evt; + HANDLE ph, connect_evt; fhandler_base *fh; ph = create_pipe_instance (true); @@ -516,16 +492,7 @@ fhandler_fifo::open (int flags, mode_t) delete fh; goto out; } - if (!(dummy_evt = create_event ())) - { - res = error_errno_set; - delete fh; - fh->close (); - CloseHandle (connect_evt); - goto out; - } - fc_handler[0] = fifo_client_handler (fh, fc_connected, connect_evt, - dummy_evt); + fc_handler[0] = fifo_client_handler (fh, fc_connected, connect_evt); nconnected = nhandlers = 1; } @@ -866,8 +833,6 @@ fifo_client_handler::close () } if (connect_evt) CloseHandle (connect_evt); - if (dummy_evt) - CloseHandle (dummy_evt); return res; } @@ -946,10 +911,6 @@ fhandler_fifo::dup (fhandler_base *child, int flags) || !DuplicateHandle (GetCurrentProcess (), fc_handler[i].connect_evt, GetCurrentProcess (), &fhf->fc_handler[i].connect_evt, - 0, true, DUPLICATE_SAME_ACCESS) - || !DuplicateHandle (GetCurrentProcess (), fc_handler[i].dummy_evt, - GetCurrentProcess (), - &fhf->fc_handler[i].dummy_evt, 0, true, DUPLICATE_SAME_ACCESS)) { CloseHandle (fhf->read_ready); @@ -978,7 +939,6 @@ fhandler_fifo::fixup_after_fork (HANDLE parent) { fc_handler[i].fh->fhandler_base::fixup_after_fork (parent); fork_fixup (parent, fc_handler[i].connect_evt, "connect_evt"); - fork_fixup (parent, fc_handler[i].dummy_evt, "dummy_evt"); } listen_client_thr = NULL; lct_termination_evt = NULL; @@ -995,6 +955,5 @@ fhandler_fifo::set_close_on_exec (bool val) { fc_handler[i].fh->fhandler_base::set_close_on_exec (val); set_no_inheritance (fc_handler[i].connect_evt, val); - set_no_inheritance (fc_handler[i].dummy_evt, val); } } -- 2.17.0