* 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 20:46:13 +00:00 · 2014-11-28 20:46:13 +00:00 · 26158dc3e9
parent c2f50c4099
commit 26158dc3e9
8 changed files with 200 additions and 67 deletions
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@ -1,3 +1,28 @@
+2014-11-28  Corinna Vinschen  <corinna@vinschen.de>
+
+	* 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  <corinna@vinschen.de>

 	* cygheap.cc (init_cygheap::find_tls): Add comment.
--- 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;
 }
--- 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); }
 };


--- 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__
--- 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;

--- 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
--- 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;
--- 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