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 | 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)
--
2.27.0
