Fix race condition when waiting for a signal

        * cygtls.h (_cygtls::wait_signal_arrived): Renamed from
        set_signal_arrived.
        (_cygtls::set_signal_arrived): New function signalling signal_arrived.
        (_cygtls::reset_signal_arrived): Don't reset will_wait_for_signal.
        (_cygtls::unwait_signal_arrived): New function only resetting
        will_wait_for_signal.
        (class wait_signal_arrived): Rename from set_signal_arrived.
        Accommodate name change throughout Cygwin.
        (wait_signal_arrived::~wait_signal_arrived): Call
        _cygtls::unwait_signal_arrived.  Add comment.
        * cygserver_ipc.h (ipc_set_proc_info): Fetch signal_arrived handle
        via call to _cygtls::get_signal_arrived.
        * exceptions.cc (_cygtls::interrupt_setup): Signal signal_arrived via
        call to _cygtls::set_signal_arrived.
        (_cygtls::handle_SIGCONT): Ditto.
        * fhandler_socket.cc (fhandler_socket::wait_for_events): Generate
        WSAEVENT array prior to entering wait loop.  Add cancel event object
        if available.  Remove calls to pthread_testcancel and just call
        pthread::static_cancel_self if the cancel event object is signalled.

Signed-off-by: Corinna Vinschen <corinna@vinschen.de>

12 files changed, 63 insertions, 26 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 0183d82c3..474bbc875 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,25 @@
+2015-11-27  Corinna Vinschen  <corinna@vinschen.de>
+
+	* cygtls.h (_cygtls::wait_signal_arrived): Renamed from
+	set_signal_arrived.
+	(_cygtls::set_signal_arrived): New function signalling signal_arrived.
+	(_cygtls::reset_signal_arrived): Don't reset will_wait_for_signal.
+	(_cygtls::unwait_signal_arrived): New function only resetting
+	will_wait_for_signal.
+	(class wait_signal_arrived): Rename from set_signal_arrived.
+	Accommodate name change throughout Cygwin.
+	(wait_signal_arrived::~wait_signal_arrived): Call
+	_cygtls::unwait_signal_arrived.  Add comment.
+	* cygserver_ipc.h (ipc_set_proc_info): Fetch signal_arrived handle
+	via call to _cygtls::get_signal_arrived.
+	* exceptions.cc (_cygtls::interrupt_setup): Signal signal_arrived via
+	call to _cygtls::set_signal_arrived.
+	(_cygtls::handle_SIGCONT): Ditto.
+	* fhandler_socket.cc (fhandler_socket::wait_for_events): Generate
+	WSAEVENT array prior to entering wait loop.  Add cancel event object
+	if available.  Remove calls to pthread_testcancel and just call
+	pthread::static_cancel_self if the cancel event object is signalled.
+
 2015-11-26  Corinna Vinschen  <corinna@vinschen.de>
 
 	* path.cc (symlink_native): Fix index when looking for colon in path.
diff --git a/winsup/cygwin/cygserver_ipc.h b/winsup/cygwin/cygserver_ipc.h
index 14495bc94..9631eacc2 100644
--- a/winsup/cygwin/cygserver_ipc.h
+++ b/winsup/cygwin/cygserver_ipc.h
@@ -43,10 +43,7 @@ ipc_set_proc_info (proc &blk, bool in_fork = false)
   blk.gidcnt = 0;
   blk.gidlist = NULL;
   blk.is_admin = false;
-  if (in_fork)
-    blk.signal_arrived = NULL;
-  else
-    _my_tls.set_signal_arrived (true, blk.signal_arrived);
+  blk.signal_arrived = in_fork ? NULL : _my_tls.get_signal_arrived (true);
 }
 #endif /* __INSIDE_CYGWIN__ */
 
diff --git a/winsup/cygwin/cygthread.cc b/winsup/cygwin/cygthread.cc
index e48a73e54..859754152 100644
--- a/winsup/cygwin/cygthread.cc
+++ b/winsup/cygwin/cygthread.cc
@@ -1,7 +1,7 @@
 /* cygthread.cc
 
    Copyright 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2008, 2009,
-   2010, 2011, 2012, 2013 Red Hat, Inc.
+   2010, 2011, 2012, 2013, 2015 Red Hat, Inc.
 
 This software is a copyrighted work licensed under the terms of the
 Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
@@ -374,7 +374,7 @@ cygthread::detach (HANDLE sigwait)
 	  unsigned n = 2;
 	  DWORD howlong = INFINITE;
 	  w4[0] = sigwait;
-	  set_signal_arrived here (w4[1]);
+	  wait_signal_arrived here (w4[1]);
 	  /* For a description of the below loop see the end of this file */
 	  for (int i = 0; i < 2; i++)
 	    switch (res = WaitForMultipleObjects (n, w4, FALSE, howlong))
diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h
index b386b989d..527147324 100644
--- a/winsup/cygwin/cygtls.h
+++ b/winsup/cygwin/cygtls.h
@@ -248,7 +248,7 @@ public:
       }
     return signal_arrived;
   }
-  void set_signal_arrived (bool setit, HANDLE& h)
+  void wait_signal_arrived (bool setit, HANDLE& h)
   {
     if (!setit)
       will_wait_for_signal = false;
@@ -258,10 +258,17 @@ public:
 	will_wait_for_signal = true;
       }
   }
+  void set_signal_arrived ()
+  {
+    SetEvent (get_signal_arrived (false));
+  }
   void reset_signal_arrived ()
   {
     if (signal_arrived)
       ResetEvent (signal_arrived);
+  }
+  void unwait_signal_arrived ()
+  {
     will_wait_for_signal = false;
   }
   void handle_SIGCONT ();
@@ -430,14 +437,18 @@ public:
   }
 #endif /* __x86_64__ */
 
-class set_signal_arrived
+class wait_signal_arrived
 {
 public:
-  set_signal_arrived (bool setit, HANDLE& h) { _my_tls.set_signal_arrived (setit, h); }
-  set_signal_arrived (HANDLE& h) { _my_tls.set_signal_arrived (true, h); }
+  wait_signal_arrived (bool setit, HANDLE& h) { _my_tls.wait_signal_arrived (setit, h); }
+  wait_signal_arrived (HANDLE& h) { _my_tls.wait_signal_arrived (true, h); }
 
   operator int () const {return _my_tls.will_wait_for_signal;}
-  ~set_signal_arrived () { _my_tls.reset_signal_arrived (); }
+  /* Do not reset the signal_arrived event just because we leave the scope of
+     this wait_signal_arrived object.  This may lead to all sorts of races.
+     The only method actually resetting the signal_arrived event is
+     _cygtls::call_signal_handler. */
+  ~wait_signal_arrived () { _my_tls.unwait_signal_arrived (); }
 };
 
 #define __getreent() (&_my_tls.local_clib)
diff --git a/winsup/cygwin/cygwait.cc b/winsup/cygwin/cygwait.cc
index 71d30d164..3fc9a3832 100644
--- a/winsup/cygwin/cygwait.cc
+++ b/winsup/cygwin/cygwait.cc
@@ -40,7 +40,7 @@ cygwait (HANDLE object, PLARGE_INTEGER timeout, unsigned mask)
   if (object)
     wait_objects[num++] = object;
 
-  set_signal_arrived thread_waiting (is_cw_sig_handle, wait_objects[num]);
+  wait_signal_arrived thread_waiting (is_cw_sig_handle, wait_objects[num]);
   debug_only_printf ("object %p, thread waiting %d, signal_arrived %p", object, (int) thread_waiting, _my_tls.signal_arrived);
   DWORD sig_n;
   if (!thread_waiting)
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 9119a8c08..c3a45d288 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -972,7 +972,7 @@ _cygtls::interrupt_setup (siginfo_t& si, void *handler, struct sigaction& siga)
   this->sig = si.si_signo; /* Should always be last thing set to avoid race */
 
   if (incyg)
-    SetEvent (get_signal_arrived (false));
+    set_signal_arrived ();
 
   if (!have_execed)
     proc_subproc (PROC_CLEARWAIT, 1);
@@ -1404,7 +1404,7 @@ _cygtls::handle_SIGCONT ()
     else
       {
 	sig = SIGCONT;
-	SetEvent (signal_arrived); /* alert sig_handle_tty_stop */
+	set_signal_arrived (); /* alert sig_handle_tty_stop */
 	sigsent = true;
       }
   /* Clear pending stop signals */
diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc
index bc3c610e0..21bc7316e 100644
--- a/winsup/cygwin/fhandler_socket.cc
+++ b/winsup/cygwin/fhandler_socket.cc
@@ -748,6 +748,12 @@ fhandler_socket::wait_for_events (const long event_mask, const DWORD flags)
   int ret;
   long events = 0;
 
+  WSAEVENT ev[3] = { wsock_evt, NULL, NULL };
+  wait_signal_arrived here (ev[1]);
+  DWORD ev_cnt = 2;
+  if ((ev[2] = pthread::get_cancel_event ()) != NULL)
+    ++ev_cnt;
+
   while (!(ret = evaluate_events (event_mask, events, !(flags & MSG_PEEK)))
 	 && !events)
     {
@@ -757,14 +763,9 @@ fhandler_socket::wait_for_events (const long event_mask, const DWORD flags)
 	  return SOCKET_ERROR;
 	}
 
-      WSAEVENT ev[2] = { wsock_evt };
-      set_signal_arrived here (ev[1]);
-      switch (WSAWaitForMultipleEvents (2, ev, FALSE, 50, FALSE))
+      switch (WSAWaitForMultipleEvents (ev_cnt, ev, FALSE, 50, FALSE))
 	{
 	  case WSA_WAIT_TIMEOUT:
-	    pthread_testcancel ();
-	    break;
-
 	  case WSA_WAIT_EVENT_0:
 	    break;
 
@@ -774,8 +775,11 @@ fhandler_socket::wait_for_events (const long event_mask, const DWORD flags)
 	    WSASetLastError (WSAEINTR);
 	    return SOCKET_ERROR;
 
+	  case WSA_WAIT_EVENT_0 + 2:
+	    pthread::static_cancel_self ();
+	    break;
+
 	  default:
-	    pthread_testcancel ();
 	    /* wsock_evt can be NULL.  We're generating the same errno values
 	       as for sockets on which shutdown has been called. */
 	    if (WSAGetLastError () != WSA_INVALID_HANDLE)
diff --git a/winsup/cygwin/fhandler_windows.cc b/winsup/cygwin/fhandler_windows.cc
index 5cafe13d9..a25812748 100644
--- a/winsup/cygwin/fhandler_windows.cc
+++ b/winsup/cygwin/fhandler_windows.cc
@@ -1,7 +1,7 @@
 /* fhandler_windows.cc: code to access windows message queues.
 
    Copyright 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2009, 2011, 2012,
-   2013 Red Hat, Inc.
+   2013, 2015 Red Hat, Inc.
 
    Written by Sergey S. Okhapkin (sos@prospect.com.ru).
    Feedback and testing by Andy Piper (andyp@parallax.co.uk).
@@ -92,7 +92,7 @@ fhandler_windows::read (void *buf, size_t& len)
     }
 
   HANDLE w4[2];
-  set_signal_arrived here (w4[0]);
+  wait_signal_arrived here (w4[0]);
   DWORD cnt = 1;
   if ((w4[1] = pthread::get_cancel_event ()) != NULL)
     ++cnt;
diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc
index f7c04c8e1..d0fea282a 100644
--- a/winsup/cygwin/flock.cc
+++ b/winsup/cygwin/flock.cc
@@ -1300,7 +1300,7 @@ lf_setlock (lockf_t *lock, inode_t *node, lockf_t **clean, HANDLE fhdl)
 	timeout = 100L;
 
       DWORD WAIT_SIGNAL_ARRIVED = WAIT_OBJECT_0 + wait_count;
-      set_signal_arrived here (w4[wait_count++]);
+      wait_signal_arrived here (w4[wait_count++]);
 
       DWORD WAIT_THREAD_CANCELED = WAIT_TIMEOUT + 1;
       HANDLE cancel_event = pthread::get_cancel_event ();
diff --git a/winsup/cygwin/posix_ipc.cc b/winsup/cygwin/posix_ipc.cc
index ef05dbc1c..e82c38526 100644
--- a/winsup/cygwin/posix_ipc.cc
+++ b/winsup/cygwin/posix_ipc.cc
@@ -178,7 +178,7 @@ ipc_cond_timedwait (HANDLE evt, HANDLE mtx, const struct timespec *abstime)
   DWORD timer_idx = 0;
   int ret = 0;
 
-  set_signal_arrived here (w4[1]);
+  wait_signal_arrived here (w4[1]);
   if ((w4[cnt] = pthread::get_cancel_event ()) != NULL)
     ++cnt;
   if (abstime)
diff --git a/winsup/cygwin/release/2.4.0 b/winsup/cygwin/release/2.4.0
index bb47216d2..cb1b069cf 100644
--- a/winsup/cygwin/release/2.4.0
+++ b/winsup/cygwin/release/2.4.0
@@ -41,3 +41,6 @@ Bug Fixes
 - Replaced old, buggy strtold implementation with well-tested gdtoa version
   from David M. Gay.
   Addresses: https://cygwin.com/ml/cygwin/2015-11/msg00205.html
+
+- Fix a race condition in signal handling.
+  Addresses: https://cygwin.com/ml/cygwin/2015-11/msg00387.html
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 803e5d5f5..5409205be 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -353,7 +353,7 @@ select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   select_record *s = &start;
   DWORD m = 0;
 
-  set_signal_arrived here (w4[m++]);
+  wait_signal_arrived here (w4[m++]);
   if ((w4[m] = pthread::get_cancel_event ()) != NULL)
     m++;