From da9fea07592987474203a531b88d83312f569141 Mon Sep 17 00:00:00 2001 From: Ken Brown Date: Wed, 15 Jul 2020 09:46:42 -0400 Subject: [PATCH] Cygwin: FIFO: fix problems finding new owner When the owning reader closes and there are still readers open, the owner needs to wait for a new owner to be found before closing its fifo_client handlers. This involves a loop in which dec_nreaders is called at the beginning and inc_nreaders is called at the end. Any other reader that tries to access shmem->_nreaders during this loop will therefore get an inaccurate answer. Fix this by adding an nreaders method and using it instead of dec_nreaders and inc_nreaders. Also add nreaders_lock to control access to the shmem->_nreaders. Make various other changes to improve the reliability of finding a new owner. --- winsup/cygwin/fhandler.h | 8 ++- winsup/cygwin/fhandler_fifo.cc | 90 +++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 35 deletions(-) diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h index 7a28adc16..cf6daea06 100644 --- a/winsup/cygwin/fhandler.h +++ b/winsup/cygwin/fhandler.h @@ -1328,7 +1328,7 @@ class fifo_shmem_t { LONG _nreaders; fifo_reader_id_t _owner, _prev_owner, _pending_owner; - af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock; + af_unix_spinlock_t _owner_lock, _reading_lock, _reader_opening_lock, _nreaders_lock; /* Info about shared memory block used for temporary storage of the owner's fc_handler list. */ @@ -1336,6 +1336,7 @@ class fifo_shmem_t _sh_fc_handler_updated; public: + int nreaders () const { return (int) _nreaders; } int inc_nreaders () { return (int) InterlockedIncrement (&_nreaders); } int dec_nreaders () { return (int) InterlockedDecrement (&_nreaders); } @@ -1352,6 +1353,8 @@ public: void reading_unlock () { _reading_lock.unlock (); } void reader_opening_lock () { _reader_opening_lock.lock (); } void reader_opening_unlock () { _reader_opening_lock.unlock (); } + void nreaders_lock () { _nreaders_lock.lock (); } + void nreaders_unlock () { _nreaders_lock.unlock (); } int get_shared_nhandlers () const { return (int) _sh_nhandlers; } void set_shared_nhandlers (int n) { InterlockedExchange (&_sh_nhandlers, n); } @@ -1420,8 +1423,11 @@ class fhandler_fifo: public fhandler_base int reopen_shared_fc_handler (); int remap_shared_fc_handler (size_t); + int nreaders () const { return shmem->nreaders (); } int inc_nreaders () { return shmem->inc_nreaders (); } int dec_nreaders () { return shmem->dec_nreaders (); } + void nreaders_lock () { shmem->nreaders_lock (); } + void nreaders_unlock () { shmem->nreaders_unlock (); } fifo_reader_id_t get_prev_owner () const { return shmem->get_prev_owner (); } void set_prev_owner (fifo_reader_id_t fr_id) diff --git a/winsup/cygwin/fhandler_fifo.cc b/winsup/cygwin/fhandler_fifo.cc index 3d34cdfab..2d4f7a97e 100644 --- a/winsup/cygwin/fhandler_fifo.cc +++ b/winsup/cygwin/fhandler_fifo.cc @@ -371,6 +371,8 @@ fhandler_fifo::record_connection (fifo_client_handler& fc, int fhandler_fifo::update_my_handlers () { + int ret = 0; + close_all_handlers (); fifo_reader_id_t prev = get_prev_owner (); if (!prev) @@ -387,7 +389,7 @@ fhandler_fifo::update_my_handlers () { debug_printf ("Can't open process of previous owner, %E"); __seterrno (); - return -1; + goto out; } for (int i = 0; i < get_shared_nhandlers (); i++) @@ -402,11 +404,13 @@ fhandler_fifo::update_my_handlers () debug_printf ("Can't duplicate handle of previous owner, %E"); --nhandlers; __seterrno (); - return -1; + goto out; } fc.state = shared_fc_handler[i].state; } - return 0; +out: + set_prev_owner (null_fr_id); + return ret; } int @@ -1414,41 +1418,59 @@ fhandler_fifo::close () { if (reader) { - /* If we're the owner, try to find a new owner. */ - bool find_new_owner = false; + /* If we're the owner, we can't close our fc_handlers if a new + owner might need to duplicate them. */ + bool close_fc_ok = false; cancel_reader_thread (); - owner_lock (); - if (get_owner () == me) - { - find_new_owner = true; - set_owner (null_fr_id); - set_prev_owner (me); - owner_needed (); - } - owner_unlock (); + nreaders_lock (); if (dec_nreaders () == 0) - ResetEvent (read_ready); - else if (find_new_owner && !IsEventSignalled (owner_found_evt)) { - bool found = false; - do - if (dec_nreaders () >= 0) - { - /* There's still another reader open. */ - if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0) - found = true; - else - { - owner_lock (); - if (get_owner ()) /* We missed owner_found_evt? */ - found = true; - else - owner_needed (); - owner_unlock (); - } - } - while (inc_nreaders () > 0 && !found); + close_fc_ok = true; + ResetEvent (read_ready); + ResetEvent (owner_needed_evt); + ResetEvent (owner_found_evt); + set_owner (null_fr_id); + set_prev_owner (null_fr_id); + set_pending_owner (null_fr_id); + set_shared_nhandlers (0); + } + else + { + owner_lock (); + if (get_owner () != me) + close_fc_ok = true; + else + { + set_owner (null_fr_id); + set_prev_owner (me); + if (!get_pending_owner ()) + owner_needed (); + } + owner_unlock (); + } + nreaders_unlock (); + while (!close_fc_ok) + { + if (WaitForSingleObject (owner_found_evt, 1) == WAIT_OBJECT_0) + close_fc_ok = true; + else + { + nreaders_lock (); + if (!nreaders ()) + { + close_fc_ok = true; + nreaders_unlock (); + } + else + { + nreaders_unlock (); + owner_lock (); + if (get_owner () || get_prev_owner () != me) + close_fc_ok = true; + owner_unlock (); + } + } } close_all_handlers (); if (fc_handler)