[PATCH 6/8] Cygwin: FIFO: synchronize the fifo_reader and fifosel threads
Ken Brown
kbrown@cornell.edu
Tue Aug 4 12:55:05 GMT 2020
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 ());
--
2.28.0
More information about the Cygwin-patches
mailing list