diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index 3eb802f39..8975e68fc 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,28 @@ +2014-11-28 Corinna Vinschen + + * cygheap.cc (init_cygheap::init_tls_list): Accommodate threadlist + having a new type threadlist_t *. Convert commented out code into an + #if 0. Create thread mutex. Explain why. + (init_cygheap::remove_tls): Drop timeout value. Always wait infinitely + for tls_sentry. Return mutex HANDLE of just deleted threadlist entry. + (init_cygheap::find_tls): New implementation taking tls pointer as + search parameter. Return threadlist_t *. + (init_cygheap::find_tls): Return threadlist_t *. Define ix as auto + variable. Drop exception handling since crash must be made impossible + due to correct synchronization. Return with locked mutex. + * cygheap.h (struct threadlist_t): Define. + (struct init_cygheap): Convert threadlist to threadlist_t type. + (init_cygheap::remove_tls): Align declaration to above change. + (init_cygheap::find_tls): Ditto. + (init_cygheap::unlock_tls): Define. + * cygtls.cc (_cygtls::remove): Unlock and close mutex when finishing. + * exceptions.cc (sigpacket::process): Lock _cygtls area of thread before + accessing it. + * fhandler_termios.cc (fhandler_termios::bg_check): Ditto. + * sigproc.cc (sig_send): Ditto. + * thread.cc (pthread::exit): Ditto. Add comment. + (pthread::cancel): Ditto. + 2014-11-28 Corinna Vinschen * cygheap.cc (init_cygheap::find_tls): Add comment. diff --git a/winsup/cygwin/cygheap.cc b/winsup/cygwin/cygheap.cc index 594f53c9c..3077e0688 100644 --- a/winsup/cygwin/cygheap.cc +++ b/winsup/cygwin/cygheap.cc @@ -617,8 +617,9 @@ init_cygheap::init_tls_list () else { sthreads = THREADLIST_CHUNK; - threadlist = (_cygtls **) ccalloc_abort (HEAP_TLS, cygheap->sthreads, - sizeof (cygheap->threadlist[0])); + threadlist = (threadlist_t *) + ccalloc_abort (HEAP_TLS, cygheap->sthreads, + sizeof (cygheap->threadlist[0])); } tls_sentry::lock.init ("thread_tls_sentry"); } @@ -630,72 +631,116 @@ init_cygheap::add_tls (_cygtls *t) tls_sentry here (INFINITE); if (nthreads >= cygheap->sthreads) { - threadlist = (_cygtls **) + threadlist = (threadlist_t *) crealloc_abort (threadlist, (sthreads += THREADLIST_CHUNK) * sizeof (threadlist[0])); - // memset (threadlist + nthreads, 0, THREADLIST_CHUNK * sizeof (threadlist[0])); +#if 0 + memset (threadlist + nthreads, 0, + THREADLIST_CHUNK * sizeof (threadlist[0])); +#endif } - threadlist[nthreads++] = t; + /* Create a mutex to lock the thread's _cygtls area. This is required for + the following reason: The thread's _cygtls area is on the thread's + own stack. Thus, when the thread exits, its _cygtls area is automatically + destroyed by the OS. Thus, when this happens while the signal thread + still utilizes the thread's _cygtls area, things go awry. + + The following methods take this into account: + + - The thread mutex is generally only locked under tls_sentry locking. + - remove_tls, called from _cygtls::remove, locks the mutex before + removing the threadlist entry and _cygtls::remove then unlocks and + destroyes the mutex. + - find_tls, called from several places but especially from the signal + thread, will lock the mutex on exit and the caller can access the + _cygtls area locked. Always make sure to unlock the mutex when the + _cygtls area isn't needed anymore. */ + threadlist[nthreads].thread = t; + threadlist[nthreads].mutex = CreateMutexW (&sec_none_nih, FALSE, NULL); + if (!threadlist[nthreads].mutex) + api_fatal ("Can't create per-thread mutex, %E"); + ++nthreads; } -void -init_cygheap::remove_tls (_cygtls *t, DWORD wait) +HANDLE __reg3 +init_cygheap::remove_tls (_cygtls *t) { - tls_sentry here (wait); + HANDLE mutex = NULL; + + tls_sentry here (INFINITE); if (here.acquired ()) { for (uint32_t i = 0; i < nthreads; i++) - if (t == threadlist[i]) + if (t == threadlist[i].thread) { + mutex = threadlist[i].mutex; + WaitForSingleObject (mutex, INFINITE); if (i < --nthreads) threadlist[i] = threadlist[nthreads]; debug_only_printf ("removed %p element %u", this, i); break; } } + /* Leave with locked mutex. The calling function is responsible for + unlocking the mutex. */ + return mutex; } -_cygtls __reg3 * +threadlist_t __reg2 * +init_cygheap::find_tls (_cygtls *tls) +{ + tls_sentry here (INFINITE); + + threadlist_t *t = NULL; + int ix = -1; + while (++ix < (int) nthreads) + { + if (!threadlist[ix].thread->tid + || !threadlist[ix].thread->initialized) + ; + if (threadlist[ix].thread == tls) + { + t = &threadlist[ix]; + break; + } + } + /* Leave with locked mutex. The calling function is responsible for + unlocking the mutex. */ + if (t) + WaitForSingleObject (t->mutex, INFINITE); + return t; +} + +threadlist_t __reg3 * init_cygheap::find_tls (int sig, bool& issig_wait) { debug_printf ("sig %d\n", sig); tls_sentry here (INFINITE); - static int NO_COPY ix; - - _cygtls *t = NULL; + threadlist_t *t = NULL; issig_wait = false; - ix = -1; + int ix = -1; /* Scan thread list looking for valid signal-delivery candidates */ while (++ix < (int) nthreads) { - __try + /* Only pthreads have tid set to non-0. */ + if (!threadlist[ix].thread->tid + || !threadlist[ix].thread->initialized) + ; + else if (sigismember (&(threadlist[ix].thread->sigwait_mask), sig)) { - /* Only pthreads have tid set to non-0. */ - if (!threadlist[ix]->tid) - continue; - else if (sigismember (&(threadlist[ix]->sigwait_mask), sig)) - { - t = cygheap->threadlist[ix]; - issig_wait = true; - __leave; - } - else if (!t && !sigismember (&(threadlist[ix]->sigmask), sig)) - t = cygheap->threadlist[ix]; + t = &cygheap->threadlist[ix]; + issig_wait = true; + break; } - __except (NO_ERROR) - { - /* We're here because threadlist[ix] became NULL or invalid. In - theory this should be impossible due to correct synchronization. - But *if* it happens, just remove threadlist[ix] from threadlist. - TODO: This should be totally unnecessary. */ - debug_printf ("cygtls synchronization is leaking..."); - remove_tls (threadlist[ix], 0); - --ix; - } - __endtry + else if (!t && !sigismember (&(threadlist[ix].thread->sigmask), sig)) + t = &cygheap->threadlist[ix]; } + /* Leave with locked mutex. The calling function is responsible for + unlocking the mutex. */ + if (t) + WaitForSingleObject (t->mutex, INFINITE); return t; } diff --git a/winsup/cygwin/cygheap.h b/winsup/cygwin/cygheap.h index 1502e8c43..fcdf554a0 100644 --- a/winsup/cygwin/cygheap.h +++ b/winsup/cygwin/cygheap.h @@ -534,6 +534,14 @@ struct mini_cygheap #define NBUCKETS 40 +struct threadlist_t +{ + struct _cygtls *thread; + HANDLE mutex; /* Used to avoid accessing tls area of + deleted thread. See comment in + cygheap::remove_tls for a description. */ +}; + struct init_cygheap: public mini_cygheap { _cmalloc_entry *chain; @@ -561,7 +569,7 @@ struct init_cygheap: public mini_cygheap struct sigaction *sigs; fhandler_termios *ctty; /* Current tty */ - struct _cygtls **threadlist; + threadlist_t *threadlist; uint32_t sthreads; pid_t pid; /* my pid */ struct { /* Equivalent to using LIST_HEAD. */ @@ -572,8 +580,10 @@ struct init_cygheap: public mini_cygheap void init_installation_root (); void __reg1 init_tls_list ();; void __reg2 add_tls (_cygtls *); - void __reg3 remove_tls (_cygtls *, DWORD); - _cygtls __reg3 *find_tls (int, bool&); + HANDLE __reg3 remove_tls (_cygtls *); + threadlist_t __reg2 *find_tls (_cygtls *); + threadlist_t __reg3 *find_tls (int, bool&); + void unlock_tls (threadlist_t *t) { if (t) ReleaseMutex (t->mutex); } }; diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc index 83a62f1ae..84170ca01 100644 --- a/winsup/cygwin/cygtls.cc +++ b/winsup/cygwin/cygtls.cc @@ -180,7 +180,7 @@ _cygtls::remove (DWORD wait) debug_printf ("wait %u", wait); - cygheap->remove_tls (this, INFINITE); + HANDLE mutex = cygheap->remove_tls (this); remove_wq (wait); /* FIXME: Need some sort of atthreadexit function to allow things like @@ -211,6 +211,11 @@ _cygtls::remove (DWORD wait) /* Close timer handle. */ if (locals.cw_timer) NtClose (locals.cw_timer); + if (mutex) + { + ReleaseMutex (mutex); + CloseHandle (mutex); + } } #ifdef __x86_64__ diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc index 4e9c77036..c16b073c6 100644 --- a/winsup/cygwin/exceptions.cc +++ b/winsup/cygwin/exceptions.cc @@ -1299,6 +1299,9 @@ sigpacket::process () struct sigaction& thissig = global_sigs[si.si_signo]; void *handler = have_execed ? NULL : (void *) thissig.sa_handler; + threadlist_t *tl_entry = NULL; + _cygtls *tls = NULL; + /* Don't try to send signals if we're just starting up since signal masks may not be available. */ if (!cygwin_finished_initializing) @@ -1311,12 +1314,19 @@ sigpacket::process () myself->rusage_self.ru_nsignals++; - _cygtls *tls; if (si.si_signo == SIGCONT) - _main_tls->handle_SIGCONT (); + { + tl_entry = cygheap->find_tls (_main_tls); + _main_tls->handle_SIGCONT (); + cygheap->unlock_tls (tl_entry); + } + /* SIGKILL is special. It always goes through. */ if (si.si_signo == SIGKILL) - tls = _main_tls; /* SIGKILL is special. It always goes through. */ + { + tl_entry = cygheap->find_tls (_main_tls); + tls = _main_tls; + } else if (ISSTATE (myself, PID_STOPPED)) { rc = -1; /* Don't send signals when stopped */ @@ -1324,18 +1334,29 @@ sigpacket::process () } else if (!sigtls) { - tls = cygheap->find_tls (si.si_signo, issig_wait); - sigproc_printf ("using tls %p", tls); + tl_entry = cygheap->find_tls (si.si_signo, issig_wait); + if (tl_entry) + { + tls = tl_entry->thread; + sigproc_printf ("using tls %p", tls); + } } else { - tls = sigtls; - if (sigismember (&tls->sigwait_mask, si.si_signo)) - issig_wait = true; - else if (!sigismember (&tls->sigmask, si.si_signo)) - issig_wait = false; - else - tls = NULL; + tl_entry = cygheap->find_tls (sigtls); + if (tl_entry) + { + tls = tl_entry->thread; + if (sigismember (&tls->sigwait_mask, si.si_signo)) + issig_wait = true; + else if (!sigismember (&tls->sigmask, si.si_signo)) + issig_wait = false; + else + { + cygheap->unlock_tls (tl_entry); + tls = NULL; + } + } } /* !tls means no threads available to catch a signal. */ @@ -1371,19 +1392,22 @@ sigpacket::process () } /* Clear pending SIGCONT on stop signals */ - if (si.si_signo == SIGTSTP || si.si_signo == SIGTTIN || si.si_signo == SIGTTOU) + if (si.si_signo == SIGTSTP || si.si_signo == SIGTTIN + || si.si_signo == SIGTTOU) sig_clear (SIGCONT); if (handler == (void *) SIG_DFL) { - if (si.si_signo == SIGCHLD || si.si_signo == SIGIO || si.si_signo == SIGCONT || si.si_signo == SIGWINCH + if (si.si_signo == SIGCHLD || si.si_signo == SIGIO + || si.si_signo == SIGCONT || si.si_signo == SIGWINCH || si.si_signo == SIGURG) { sigproc_printf ("signal %d default is currently ignore", si.si_signo); goto done; } - if (si.si_signo == SIGTSTP || si.si_signo == SIGTTIN || si.si_signo == SIGTTOU) + if (si.si_signo == SIGTSTP || si.si_signo == SIGTTIN + || si.si_signo == SIGTTOU) goto stop; goto exit_sig; @@ -1395,7 +1419,12 @@ sigpacket::process () goto dosig; stop: - tls = _main_tls; + if (tls != _main_tls) + { + cygheap->unlock_tls (tl_entry); + tl_entry = cygheap->find_tls (_main_tls); + tls = _main_tls; + } handler = (void *) sig_handle_tty_stop; thissig = global_sigs[SIGSTOP]; goto dosig; @@ -1415,6 +1444,7 @@ dosig: rc = setup_handler (handler, thissig, tls); done: + cygheap->unlock_tls (tl_entry); sigproc_printf ("returning %d", rc); return rc; diff --git a/winsup/cygwin/fhandler_termios.cc b/winsup/cygwin/fhandler_termios.cc index 1786e49c9..6a4d666a0 100644 --- a/winsup/cygwin/fhandler_termios.cc +++ b/winsup/cygwin/fhandler_termios.cc @@ -193,9 +193,12 @@ fhandler_termios::bg_check (int sig) return bg_eof; } + threadlist_t *tl_entry; + tl_entry = cygheap->find_tls (_main_tls); int sigs_ignored = ((void *) global_sigs[sig].sa_handler == (void *) SIG_IGN) || (_main_tls->sigmask & SIGTOMASK (sig)); + cygheap->unlock_tls (tl_entry); /* If the process is ignoring SIGTT*, then background IO is OK. If the process is not ignoring SIGTT*, then the sig is to be sent to diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index c53efccc1..12f61d2fb 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -608,7 +608,11 @@ sig_send (_pinfo *p, siginfo_t& si, _cygtls *tls) else if (si.si_signo == __SIGPENDING) pack.mask = &pending; else if (si.si_signo == __SIGFLUSH || si.si_signo > 0) - pack.mask = tls ? &tls->sigmask : &_main_tls->sigmask; + { + threadlist_t *tl_entry = cygheap->find_tls (tls ? tls : _main_tls); + pack.mask = tls ? &tls->sigmask : &_main_tls->sigmask; + cygheap->unlock_tls (tl_entry); + } else pack.mask = NULL; @@ -1259,9 +1263,12 @@ wait_sig (VOID *) continue; sigset_t dummy_mask; + threadlist_t *tl_entry; if (!pack.mask) { + tl_entry = cygheap->find_tls (_main_tls); dummy_mask = _main_tls->sigmask; + cygheap->unlock_tls (tl_entry); pack.mask = &dummy_mask; } @@ -1276,11 +1283,16 @@ wait_sig (VOID *) strace.activate (false); break; case __SIGPENDING: - *pack.mask = 0; - unsigned bit; - while ((q = q->next)) - if (pack.sigtls->sigmask & (bit = SIGTOMASK (q->si.si_signo))) - *pack.mask |= bit; + { + unsigned bit; + + *pack.mask = 0; + tl_entry = cygheap->find_tls (pack.sigtls); + while ((q = q->next)) + if (pack.sigtls->sigmask & (bit = SIGTOMASK (q->si.si_signo))) + *pack.mask |= bit; + cygheap->unlock_tls (tl_entry); + } break; case __SIGHOLD: sig_held = true; diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index a2e2aeb27..1ac338b4b 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -515,7 +515,7 @@ void pthread::exit (void *value_ptr) { class pthread *thread = this; - bool is_main_tls = (cygtls == _main_tls); // Check cygtls before deleting this + _cygtls *tls = cygtls; /* Save cygtls before deleting this. */ // run cleanup handlers pop_all_cleanup_handlers (); @@ -541,15 +541,16 @@ pthread::exit (void *value_ptr) ::exit (0); else { - if (is_main_tls) + if (tls == _main_tls) { - /* FIXME: Needs locking. */ + cygheap->find_tls (tls); /* Lock _main_tls mutex. */ _cygtls *dummy = (_cygtls *) malloc (sizeof (_cygtls)); *dummy = *_main_tls; _main_tls = dummy; _main_tls->initialized = 0; } - cygtls->remove (INFINITE); + /* This also unlocks and closes the _main_tls mutex. */ + tls->remove (INFINITE); ExitThread (0); } } @@ -595,6 +596,7 @@ pthread::cancel () and tends to hang infinitely if we change the instruction pointer. So just don't cancel asynchronously if the thread is currently executing Windows code. Rely on deferred cancellation in this case. */ + threadlist_t *tl_entry = cygheap->find_tls (cygtls); if (!cygtls->inside_kernel (&context)) { #ifdef __x86_64__ @@ -604,6 +606,7 @@ pthread::cancel () #endif SetThreadContext (win32_obj_id, &context); } + cygheap->unlock_tls (tl_entry); } mutex.unlock (); /* See above. For instance, a thread which waits for a semaphore in sem_wait