diff options
author | Ken Brown <kbrown@cornell.edu> | 2020-07-15 16:46:42 +0300 |
---|---|---|
committer | Ken Brown <kbrown@cornell.edu> | 2020-07-16 22:59:53 +0300 |
commit | da9fea07592987474203a531b88d83312f569141 (patch) | |
tree | a7aa182273abc8d0db71b6971eb8402136ed8206 /winsup | |
parent | 0ee972d1b095bfea89499b358646e44d60aa4a4b (diff) |
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.
Diffstat (limited to 'winsup')
-rw-r--r-- | winsup/cygwin/fhandler.h | 8 | ||||
-rw-r--r-- | winsup/cygwin/fhandler_fifo.cc | 86 |
2 files changed, 61 insertions, 33 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) + nreaders_lock (); + if (dec_nreaders () == 0) { - find_new_owner = true; + close_fc_ok = true; + ResetEvent (read_ready); + ResetEvent (owner_needed_evt); + ResetEvent (owner_found_evt); set_owner (null_fr_id); - set_prev_owner (me); - owner_needed (); + set_prev_owner (null_fr_id); + set_pending_owner (null_fr_id); + set_shared_nhandlers (0); } - owner_unlock (); - if (dec_nreaders () == 0) - ResetEvent (read_ready); - else if (find_new_owner && !IsEventSignalled (owner_found_evt)) + else { - 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); + 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) |