From c43e9340f1bebe97d8e8ea683ec7f2bf911b5a85 Mon Sep 17 00:00:00 2001 From: Corinna Vinschen Date: Fri, 27 Nov 2015 14:39:11 +0100 Subject: [PATCH] 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 --- 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 0183d82c3..474bbc875 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,25 @@ +2015-11-27 Corinna Vinschen + + * 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 * 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++;