From 0fda55133a82eb70e98e636d0d00f48c674b9440 Mon Sep 17 00:00:00 2001 From: Ken Brown Date: Mon, 3 Aug 2020 09:38:08 -0400 Subject: [PATCH] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads The fifo_reader thread function and the function select.cc:peek_fifo() can both change the state of a fifo_client_handler. These changes are made under fifo_client_lock, so there is no race, but the changes can still be incompatible. Add code to make sure that only one of these functions can change the state from its initial fc_listening state. Whichever function does this calls the fhandler_fifo::record_connection method, which is now public so that peek_fifo can call it. Slightly modify that method to make it suitable for being called by peek_fifo. Make a few other small changes to the fifo_reader thread function to change how it deals with the STATUS_PIPE_CLOSING value that can (rarely) be returned by NtFsControlFile. Add commentary to fhandler_fifo.cc to explain fifo_client connect states and where they can be changed. --- winsup/cygwin/fhandler.h | 4 +-- winsup/cygwin/fhandler_fifo.cc | 60 ++++++++++++++++++++++++++++++---- winsup/cygwin/select.cc | 5 ++- 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 40e201b0f..a577ca542 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1413,8 +1413,6 @@ class fhandler_fifo: public fhandler_base void cleanup_handlers (); void close_all_handlers (); void cancel_reader_thread (); - void record_connection (fifo_client_handler&, - fifo_client_connect_state = fc_connected); int create_shmem (bool only_open = false); int reopen_shmem (); @@ -1482,6 +1480,8 @@ public: DWORD fifo_reader_thread_func (); void fifo_client_lock () { _fifo_client_lock.lock (); } void fifo_client_unlock () { _fifo_client_lock.unlock (); } + void record_connection (fifo_client_handler&, bool = true, + fifo_client_connect_state = fc_connected); int take_ownership (DWORD timeout = INFINITE); void reading_lock () { shmem->reading_lock (); } diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 1e1140f53..2b829eb6c 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -37,11 +37,42 @@ "fifo_client_handler" structures, one for each writer. A fifo_client_handler contains the handle for the pipe server instance and information about the state of the connection with - the writer. Each writer holds the pipe instance's client handle. + the writer. Access to the list is controlled by a + "fifo_client_lock". The reader runs a "fifo_reader_thread" that creates new pipe instances as needed and listens for client connections. + The connection state of a fifo_client_handler has one of the + following values, in which order is important: + + fc_unknown + fc_error + fc_disconnected + fc_closing + fc_listening + fc_connected + fc_input_avail + + It can be changed in the following places: + + - It is set to fc_listening when the pipe instance is created. + + - It is set to fc_connected when the fifo_reader_thread detects + a connection. + + - It is set to a value reported by the O/S when + query_and_set_state is called. This can happen in + select.cc:peek_fifo and a couple other places. + + - It is set to fc_disconnected by raw_read when an attempt to + read yields STATUS_PIPE_BROKEN. + + - It is set to fc_error in various places when unexpected + things happen. + + State changes are always guarded by fifo_client_lock. + If there are multiple readers open, only one of them, called the "owner", maintains the fifo_client_handler list. The owner is therefore the only reader that can read at any given time. If a @@ -374,10 +405,11 @@ fhandler_fifo::cleanup_handlers () /* Always called with fifo_client_lock in place. */ void -fhandler_fifo::record_connection (fifo_client_handler& fc, +fhandler_fifo::record_connection (fifo_client_handler& fc, bool set, fifo_client_connect_state s) { - fc.set_state (s); + if (set) + fc.set_state (s); set_pipe_non_blocking (fc.h, true); } @@ -583,6 +615,11 @@ owner_listen: NTSTATUS status1; fifo_client_lock (); + if (fc.get_state () != fc_listening) + /* select.cc:peek_fifo has already recorded a connection. */ + ; + else + { switch (status) { case STATUS_SUCCESS: @@ -590,7 +627,12 @@ owner_listen: record_connection (fc); break; case STATUS_PIPE_CLOSING: - record_connection (fc, fc_closing); + debug_printf ("NtFsControlFile got STATUS_PIPE_CLOSING..."); + /* Maybe a writer already connected, wrote, and closed. + Just query the O/S. */ + fc.query_and_set_state (); + debug_printf ("...O/S reports state %d", fc.get_state ()); + record_connection (fc, false); break; case STATUS_THREAD_IS_TERMINATING: case STATUS_WAIT_1: @@ -610,17 +652,23 @@ owner_listen: record_connection (fc); break; case STATUS_PIPE_CLOSING: - record_connection (fc, fc_closing); + debug_printf ("got STATUS_PIPE_CLOSING when trying to connect bogus client..."); + fc.query_and_set_state (); + debug_printf ("...O/S reports state %d", fc.get_state ()); + record_connection (fc, false); break; default: debug_printf ("NtFsControlFile status %y after failing to connect bogus client or real client", io.Status); - fc.set_state (fc_unknown); + fc.set_state (fc_error); break; } break; default: + debug_printf ("NtFsControlFile got unexpected status %y", status); + fc.set_state (fc_error); break; } + } if (ph) NtClose (ph); if (update) diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc index 9ee305f64..43f07af43 100644 --- a/winsup/cygwin/select.cc +++ b/winsup/cygwin/select.cc @@ -877,10 +877,13 @@ peek_fifo (select_record *s, bool from_select) for (int i = 0; i < fh->get_nhandlers (); i++) { fifo_client_handler &fc = fh->get_fc_handler (i); - fc.query_and_set_state (); + fifo_client_connect_state prev_state = fc.query_and_set_state (); if (fc.get_state () >= fc_connected) { nconnected++; + if (prev_state == fc_listening) + /* The connection was not recorded by the fifo_reader_thread. */ + fh->record_connection (fc, false); if (fc.get_state () == fc_input_avail) { select_printf ("read: %s, ready for read", fh->get_name ());