This is the mail archive of the
cygwin-cvs@cygwin.com
mailing list for the Cygwin project.
[newlib-cygwin] Fix race condition when waiting for a signal
- From: Corinna Vinschen <corinna at sourceware dot org>
- To: cygwin-cvs at sourceware dot org
- Date: 27 Nov 2015 13:39:20 -0000
- Subject: [newlib-cygwin] Fix race condition when waiting for a signal
https://sourceware.org/git/gitweb.cgi?p=newlib-cygwin.git;h=c43e9340f1bebe97d8e8ea683ec7f2bf911b5a85
commit c43e9340f1bebe97d8e8ea683ec7f2bf911b5a85
Author: Corinna Vinschen <corinna@vinschen.de>
Date: Fri Nov 27 14:39:11 2015 +0100
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>
Diff:
---
winsup/cygwin/ChangeLog | 22 ++++++++++++++++++++++
winsup/cygwin/cygserver_ipc.h | 5 +----
winsup/cygwin/cygthread.cc | 4 ++--
winsup/cygwin/cygtls.h | 21 ++++++++++++++++-----
winsup/cygwin/cygwait.cc | 2 +-
winsup/cygwin/exceptions.cc | 4 ++--
winsup/cygwin/fhandler_socket.cc | 18 +++++++++++-------
winsup/cygwin/fhandler_windows.cc | 4 ++--
winsup/cygwin/flock.cc | 2 +-
winsup/cygwin/posix_ipc.cc | 2 +-
winsup/cygwin/release/2.4.0 | 3 +++
winsup/cygwin/select.cc | 2 +-
12 files changed, 63 insertions(+), 26 deletions(-)
diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 0183d82..474bbc8 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 14495bc..9631eac 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 e48a73e..8597541 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 b386b98..5271473 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 71d30d1..3fc9a38 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 9119a8c..c3a45d2 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 bc3c610..21bc731 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 5cafe13..a258127 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 f7c04c8..d0fea28 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 ef05dbc..e82c385 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 bb47216..cb1b069 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 803e5d5..5409205 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++;