From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2210) id BAB7F3857C58; Thu, 16 Jul 2020 20:00:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BAB7F3857C58 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: Ken Brown To: cygwin-cvs@sourceware.org Subject: [newlib-cygwin] Cygwin: FIFO: fix problems finding new owner X-Act-Checkin: newlib-cygwin X-Git-Author: Ken Brown X-Git-Refname: refs/heads/master X-Git-Oldrev: 0ee972d1b095bfea89499b358646e44d60aa4a4b X-Git-Newrev: da9fea07592987474203a531b88d83312f569141 Message-Id: <20200716200010.BAB7F3857C58@sourceware.org> Date: Thu, 16 Jul 2020 20:00:10 +0000 (GMT) X-BeenThere: cygwin-cvs@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component git logs List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Jul 2020 20:00:10 -0000 https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=da9fea07592987474203a531b88d83312f569141 commit da9fea07592987474203a531b88d83312f569141 Author: Ken Brown Date: Wed Jul 15 09:46:42 2020 -0400 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. Diff: --- winsup/cygwin/fhandler.h | 8 +++- 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)