diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 76c6929ba..4c6e05f32 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,18 @@ +2012-12-21 Christopher Faylor + + * DevNotes: Add entry cgf-000018. + * init.cc (dll_entry): Grab process lock before exiting to ensure that + thread doesn't exit before parent if parent is exiting. + * _cygtls.cc (_cygtls::call2): Revert previous 2012-12-21 change. + * miscfuncs.cc (thread_wrapper): Ditto. + * thread.cc (pthread::exit): Ditto. + * sigproc.cc (exit_thread): Ditto. + (wait_sig): Ditto. + * sync.cc (muto::release): Ditto. + * sync.h (muto::release): Ditto. + * sigproc.h (__SIGTHREADEXIT): Delete enum. + (exit_thread): Delete declaration. + 2012-12-21 Christopher Faylor * DevNotes: Add entry cgf-000017. diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index efa338afd..04abd99cc 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -1,3 +1,14 @@ +2012-12-21 cgf-000018 + +Re: cgf-000017 + +It occurred to me that just getting the process lock during +DLL_THREAD_DETACH in dll_entry() might be adequate to fix this +problem. It's certainly much less intrusive. + +There are potential deadlock problems with grabbing a lock in +this code, though, so this check-in will be experimental. + 2012-12-21 cgf-000017 The changes in this set are to work around the issue noted here: diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc index a94f333a2..2ff22ed98 100644 --- a/winsup/cygwin/cygtls.cc +++ b/winsup/cygwin/cygtls.cc @@ -102,7 +102,7 @@ _cygtls::call2 (DWORD (*func) (void *, void *), void *arg, void *buf) dynamically loaded. */ if ((void *) func != (void *) dll_crt0_1 && (void *) func != (void *) dll_dllcrt0_1) - exit_thread (res); + ExitThread (res); } void diff --git a/winsup/cygwin/init.cc b/winsup/cygwin/init.cc index 1b245f581..d6cfb8868 100644 --- a/winsup/cygwin/init.cc +++ b/winsup/cygwin/init.cc @@ -13,6 +13,7 @@ details. */ #include "cygtls.h" #include "ntdll.h" #include "shared_info.h" +#include "sync.h" static DWORD _my_oldfunc; @@ -95,7 +96,14 @@ dll_entry (HANDLE h, DWORD reason, void *static_load) if (dll_finished_loading && (PVOID) &_my_tls > (PVOID) &test_stack_marker && _my_tls.isinitialized ()) - _my_tls.remove (0); + { + _my_tls.remove (0); + /* Make sure that we don't exit until the process has exited. + Otherwise there is a potential race where the thread exit + code could be considered to be the process exit code. + See cgf-000017 and cgf-000018 in DevNotes. */ + lock_process here; + } /* Windows 2000 has a bug in NtTerminateThread. Instead of releasing the stack at teb->DeallocationStack it uses the value of teb->Tib.StackLimit to evaluate the stack address. So we just claim diff --git a/winsup/cygwin/miscfuncs.cc b/winsup/cygwin/miscfuncs.cc index 3d76ed823..5f0625447 100644 --- a/winsup/cygwin/miscfuncs.cc +++ b/winsup/cygwin/miscfuncs.cc @@ -27,7 +27,6 @@ details. */ #include "dtable.h" #include "cygheap.h" #include "pinfo.h" -#include "sigproc.h" #include "exception.h" long tls_ix = -1; @@ -547,7 +546,7 @@ thread_wrapper (VOID *arg) : : [WRAPPER_ARG] "r" (&wrapper_arg), [CYGTLS] "i" (CYGTLS_PADSIZE)); /* Never return from here. */ - exit_thread (0); + ExitThread (0); } HANDLE WINAPI diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index be89d8685..a08a55ed3 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -553,33 +553,6 @@ sigproc_terminate (exit_states es) } } -/* Exit the current thread very carefully. - See cgf-000017 in DevNotes for more details on why this is - necessary. */ -void -exit_thread (DWORD res) -{ - HANDLE h; - - if (!DuplicateHandle (GetCurrentProcess (), GetCurrentThread (), - GetCurrentProcess (), &h, - 0, FALSE, DUPLICATE_SAME_ACCESS)) - { -#ifdef DEBUGGING - system_printf ("couldn't duplicate the current thread, %E"); -#endif - ExitThread (res); - } - ProtectHandle1 (h, exit_thread); - siginfo_t si = {__SIGTHREADEXIT, SI_KERNEL}; - si.si_value.sival_ptr = h; - /* 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); - lock_process for_now; - ExitThread (0); /* Should never hit this */ -} - int __stdcall sig_send (_pinfo *p, int sig, _cygtls *tid) { @@ -1446,23 +1419,6 @@ wait_sig (VOID *) case __SIGSETPGRP: init_console_handler (true); break; - case __SIGTHREADEXIT: - { - /* Serialize thread exit as the thread exit code can be interpreted - as the process exit code in some cases when racing with - ExitProcess/TerminateProcess. - So, wait for the thread which sent this signal to exit, then - release the process lock which it held and close it's handle. - See cgf-000017 in DevNotes for more details. - */ - HANDLE h = (HANDLE) pack.si.si_value.sival_ptr; - DWORD res = WaitForSingleObject (h, 5000); - lock_process::force_release (pack.sigtls); - ForceCloseHandle1 (h, exit_thread); - if (res != WAIT_OBJECT_0) - system_printf ("WaitForSingleObject(%p) for thread exit returned %u", h, res); - } - break; default: if (pack.si.si_signo < 0) sig_clear (-pack.si.si_signo); @@ -1505,8 +1461,5 @@ wait_sig (VOID *) close_my_readsig (); sigproc_printf ("signal thread exiting"); - /* 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. */ - Sleep (INFINITE); + ExitThread (0); } diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h index 27f690dc1..a5a2e04c5 100644 --- a/winsup/cygwin/sigproc.h +++ b/winsup/cygwin/sigproc.h @@ -25,8 +25,7 @@ enum __SIGHOLD = -(NSIG + 7), __SIGNOHOLD = -(NSIG + 8), __SIGEXIT = -(NSIG + 9), - __SIGSETPGRP = -(NSIG + 10), - __SIGTHREADEXIT = -(NSIG + 11) + __SIGSETPGRP = -(NSIG + 10) }; #endif @@ -88,7 +87,6 @@ void __stdcall sigalloc (); int kill_pgrp (pid_t, siginfo_t&); int killsys (pid_t, int); -void exit_thread (DWORD) __attribute__ ((regparm(1), noreturn)); extern "C" void sigdelayed (); diff --git a/winsup/cygwin/sync.cc b/winsup/cygwin/sync.cc index d8b3d8f54..f3796272f 100644 --- a/winsup/cygwin/sync.cc +++ b/winsup/cygwin/sync.cc @@ -4,8 +4,7 @@ which is intended to operate similarly to a mutex but attempts to avoid making expensive calls to the kernel. - Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2008, 2009, 2010, 2011, 2012 - Red Hat, Inc. + Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2008, 2009, 2010 Red Hat, Inc. This file is part of Cygwin. @@ -110,8 +109,10 @@ muto::acquired () /* Return the muto lock. Needs to be called once per every acquire. */ int -muto::release (_cygtls *this_tls) +muto::release () { + void *this_tls = &_my_tls; + if (tls != this_tls || !visits) { SetLastError (ERROR_NOT_OWNER); /* Didn't have the lock. */ diff --git a/winsup/cygwin/sync.h b/winsup/cygwin/sync.h index d424d39ab..c9c5fb595 100644 --- a/winsup/cygwin/sync.h +++ b/winsup/cygwin/sync.h @@ -33,7 +33,7 @@ public: ~muto () #endif int acquire (DWORD ms = INFINITE) __attribute__ ((regparm (2))); /* Acquire the lock. */ - int release (_cygtls * = &_my_tls) __attribute__ ((regparm (2))); /* Release the lock. */ + int release () __attribute__ ((regparm (1))); /* Release the lock. */ bool acquired () __attribute__ ((regparm (1))); void upforgrabs () {tls = this;} // just set to an invalid address @@ -60,7 +60,6 @@ public: if (!skip_unlock) locker.release (); } - static void force_release (_cygtls *tid) {locker.release (tid);} friend class dtable; friend class fhandler_fifo; }; diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index 415ad4af7..187ea52fd 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -532,7 +532,7 @@ pthread::exit (void *value_ptr) _main_tls = dummy; _main_tls->initialized = false; } - exit_thread (0); + ExitThread (0); } }