diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 4cfece717..96471a06d 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,26 @@ +2012-12-28 Christopher Faylor + + * DevNotes: Add entry cgf-000019. + * dcrt0.cc (do_exit): Just set exit_state to ES_EVENTS_TERMINATE and + nuke call to events_terminate which just set a superfluous flag. + * sigproc.cc (signal_exit_code): New variable. + (setup_signal_exit): Define new function. + (_cygtls::signal_exit): Remove accommodations for closing the signal + pipe handle. + (exit_thread): Just sleep if we're exiting. + (wait_sig): If signal_exit_code is set, just handle bookkeeping signals + and exit ReadFile loop if there is nothing more to process. Call + signal_exit at end if signal_exit_code is non-zero. + * sigproc.h (setup_signal_exit): Declare new function. + * exceptions.cc (sigpacket::process): Use setup_signal_exit to control + exiting due to a signal. + (exception::handle): Ditto. Query exit_state rather than defunct + exit_already to determine if we are exiting. + * globals.cc (ES_SIGNAL_EXIT): New enum. + * sync.h (lock_process::release): New function for explicitly unlocking + muto. + (lock_process::~lock_process): Use release method. + 2012-12-27 Christopher Faylor * fork.cc (child_info::prefork): Fix error message formatting. diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index 04abd99cc..01293bc50 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -1,3 +1,32 @@ +2012-12-28 cgf-000019 + +(I forgot to mention that cgf-000018 was reverted. Although I never saw +a hang from this, I couldn't convince myself that one wasn't possible.) + +This fix attempts to correct a deadlock where, when a true Windows +signal arrives, Windows creates a thread which "does stuff" and attempts +to exit. In the process of exiting Cygwin grabs the process lock. If +the signal thread has seen the signal and wants to exit, it can't +because the newly-created thread now holds it. But, since the new +thread is relying on the signal thread to release its process lock, +it exits and the process lock is never released. + +To fix this, I removed calls to _cygtls::signal_exit in favor of +flagging that we were exiting by setting signal_exit_code (almost forgot +to mark that NO_COPY: that would have been fun). The new function +setup_signal_exit() now handles setting things up so that ReadFile loop +in wait_sig will do the right thing when it terminates. This function +may just Sleep indefinitely if a signal is being sent from a thread +other than the signal thread. wait_sig() was changed so that it will +essentially drop into asychronous-read-mode when a signal which exits +has been detected. The ReadFile loop is exited when we know that the +process is supposed to be exiting and there is nothing else in the +signal queue. + +Although I never actually saw this happen, exit_thread() was also +changed to release the process lock and just sleep indefintely if it is +detected that we are exiting. + 2012-12-21 cgf-000018 Re: cgf-000017 diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc index 0a2584239..b55dd9069 100644 --- a/winsup/cygwin/dcrt0.cc +++ b/winsup/cygwin/dcrt0.cc @@ -1098,10 +1098,7 @@ do_exit (int status) lock_process until_exit (true); if (exit_state < ES_EVENTS_TERMINATE) - { - exit_state = ES_EVENTS_TERMINATE; - events_terminate (); - } + exit_state = ES_EVENTS_TERMINATE; if (exit_state < ES_SIGNAL) { diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 590fcd9b5..f1f0a43f8 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -41,8 +41,6 @@ static BOOL WINAPI ctrl_c_handler (DWORD); /* This is set to indicate that we have already exited. */ -static NO_COPY int exit_already = 0; - NO_COPY static struct { unsigned int code; @@ -481,9 +479,9 @@ exception::handle (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in, void return 0; } - /* If we've already exited, don't do anything here. Returning 1 + /* If we're exiting, don't do anything here. Returning 1 tells Windows to keep looking for an exception handler. */ - if (exit_already || e->ExceptionFlags) + if (exit_state || e->ExceptionFlags) return 1; siginfo_t si = {0}; @@ -673,8 +671,7 @@ exception::handle (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in, void error_code); } - /* Flag signal + core dump */ - me.signal_exit ((cygheap->rlim_core > 0UL ? 0x80 : 0) | si.si_signo); + setup_signal_exit ((cygheap->rlim_core > 0UL ? 0x80 : 0) | si.si_signo); } si.si_addr = (si.si_signo == SIGSEGV || si.si_signo == SIGBUS @@ -1249,13 +1246,9 @@ done: return rc; exit_sig: - tls->signal_exit (si.si_signo); /* never returns */ -} - -void -events_terminate () -{ - exit_already = 1; + sigproc_printf ("setting up for exit with signal %d", si.si_signo); + setup_signal_exit (si.si_signo); + return rc; } int diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc index 387515851..e0fa3eed4 100644 --- a/winsup/cygwin/globals.cc +++ b/winsup/cygwin/globals.cc @@ -34,6 +34,7 @@ UINT system_wow64_directory_length; enum exit_states { ES_NOT_EXITING = 0, + ES_SIGNAL_EXIT, ES_EXIT_STARTING, ES_PROCESS_LOCKED, ES_EVENTS_TERMINATE, diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index 33fbb3a00..32bc9a8f2 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -61,6 +61,7 @@ _cygtls NO_COPY *_sig_tls; Static HANDLE my_sendsig; Static HANDLE my_readsig; +Static int signal_exit_code; /* Function declarations */ static int __stdcall checkstate (waitq *) __attribute__ ((regparm (1))); @@ -361,32 +362,12 @@ close_my_readsig () ForceCloseHandle1 (h, my_readsig); } -/* Cover function to `do_exit' to handle exiting even in presence of more - exceptions. We used to call exit, but a SIGSEGV shouldn't cause atexit - routines to run. */ +/* Exit due to a signal, even in presence of more exceptions. We used to just + call exit, but a SIGSEGV shouldn't cause atexit routines to run. + Should only be called from the signal thread. */ void _cygtls::signal_exit (int rc) { - HANDLE myss = my_sendsig; - my_sendsig = NULL; /* Make no_signals_allowed return true */ - - /* This code used to try to always close my_readsig but it ended up - blocking for reasons that people in google think make sense. - It's possible that it was blocking because ReadFile was still active - but it isn't clear why this only caused random hangs rather than - consistent hangs. So, for now at least, avoid closing my_readsig - unless this is the signal thread. */ - if (&_my_tls == _sig_tls) - close_my_readsig (); /* Stop any currently executing sig_sends */ - else - { - sigpacket sp = {}; - sp.si.si_signo = __SIGEXIT; - DWORD len; - /* Write a packet to the wait_sig thread which tells it to exit and - close my_readsig. */ - WriteFile (myss, &sp, sizeof (sp), &len, NULL); - } signal_debugger (rc & 0x7f); if (rc == SIGQUIT || rc == SIGABRT) @@ -553,6 +534,27 @@ sigproc_terminate (exit_states es) } } +/* Set up stuff so that the signal thread will know that we are + exiting due to a signal. */ +void +setup_signal_exit (int sig) +{ + signal_exit_code = sig; /* Tell wait_sig() that we are exiting. */ + exit_state = ES_SIGNAL_EXIT; /* Tell the rest of the world that we are exiting. */ + + if (&_my_tls != _sig_tls) + { + sigpacket sp = {}; + sp.si.si_signo = __SIGEXIT; + DWORD len; + /* Write a packet to the wait_sig thread. It will eventuall cause + the process to exit too. So just wait for that to happen after + sending the packet. */ + WriteFile (my_sendsig, &sp, sizeof (sp), &len, NULL); + Sleep (INFINITE); + } +} + /* Exit the current thread very carefully. See cgf-000017 in DevNotes for more details on why this is necessary. */ @@ -576,10 +578,16 @@ exit_thread (DWORD res) siginfo_t si = {__SIGTHREADEXIT, SI_KERNEL}; si.si_value.sival_ptr = h; lock_process for_now; /* May block indefinitely if we're exiting. */ + if (exit_state) + { + for_now.release (); + Sleep (INFINITE); + } + /* Tell wait_sig to wait for this thread to exit. It can then release the lock below and close the above-opened handle. */ sig_send (myself_nowait, si, &_my_tls); - ExitThread (0); /* Should never hit this */ + ExitThread (0); } int __stdcall @@ -1368,6 +1376,7 @@ pending_signals::next () static void WINAPI wait_sig (VOID *) { + extern int signal_exit_code; _sig_tls = &_my_tls; sig_hold = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL); @@ -1380,7 +1389,14 @@ wait_sig (VOID *) { if (pack.si.si_signo == __SIGHOLD) WaitForSingleObject (sig_hold, INFINITE); + DWORD nb; + /* If signal_exit_code is set then we are shutting down due to a signal. + We'll exit this loop iff there is nothing more in the signal queue. */ + if (signal_exit_code + && (!PeekNamedPipe (my_readsig, NULL, 0, NULL, &nb, NULL) || !nb)) + break; + pack.sigtls = NULL; if (!ReadFile (my_readsig, &pack, sizeof (pack), &nb, NULL)) break; @@ -1400,6 +1416,9 @@ wait_sig (VOID *) continue; } + if (signal_exit_code && pack.si.si_signo > 0) + continue; /* No more real signals allowed */ + sigset_t dummy_mask; if (!pack.mask) { @@ -1505,8 +1524,14 @@ wait_sig (VOID *) break; } - close_my_readsig (); sigproc_printf ("signal thread exiting"); + + my_sendsig = NULL; /* Make no_signals_allowed return true */ + close_my_readsig (); /* Cause any sig_send's to stop */ + + if (signal_exit_code) + _my_tls.signal_exit (signal_exit_code); + /* Just wait for the process to go away. Otherwise, this thread's exit value could be interpreted as the process exit value. See cgf-000017 in DevNotes for more details. */ diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h index 371641b40..26332935e 100644 --- a/winsup/cygwin/sigproc.h +++ b/winsup/cygwin/sigproc.h @@ -89,6 +89,7 @@ void __stdcall sigalloc (); int kill_pgrp (pid_t, siginfo_t&); int killsys (pid_t, int); void exit_thread (DWORD) __attribute__ ((regparm (1), noreturn)); +void setup_signal_exit (int) __attribute__ ((regparm (1))); extern "C" void sigdelayed (); diff --git a/winsup/cygwin/sync.h b/winsup/cygwin/sync.h index d424d39ab..5c6bc4b27 100644 --- a/winsup/cygwin/sync.h +++ b/winsup/cygwin/sync.h @@ -55,10 +55,15 @@ public: if (exiting && exit_state < ES_PROCESS_LOCKED) exit_state = ES_PROCESS_LOCKED; } + void release () + { + locker.release (); + skip_unlock = true; + } ~lock_process () { if (!skip_unlock) - locker.release (); + release (); } static void force_release (_cygtls *tid) {locker.release (tid);} friend class dtable;