* 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.
jeremyd2019 · Nov 28, 2014 · 26158dc · 26158dc
1 parent c2f50c4
commit 26158dc
Show file tree

Hide file tree

Showing 8 changed files with 202 additions and 69 deletions.
diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
@@ -1,3 +1,28 @@
+2014-11-28  Corinna Vinschen  <[email protected]>
+
+	* 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  <[email protected]>
 
 	* cygheap.cc (init_cygheap::find_tls): Add comment.

diff --git 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;
+}
+
+HANDLE __reg3
+init_cygheap::remove_tls (_cygtls *t)
+{
+  HANDLE mutex = NULL;
 
-void
-init_cygheap::remove_tls (_cygtls *t, DWORD wait)
-{
-  tls_sentry here (wait);
+  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]->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];
-	}
-      __except (NO_ERROR)
+      /* 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))
 	{
-	  /* 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;
+	  t = &cygheap->threadlist[ix];
+	  issig_wait = true;
+	  break;
 	}
-      __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
@@ -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
@@ -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
@@ -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,31 +1314,49 @@ 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 */
       goto done;
     }
   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 @@ sigpacket::process ()
   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
@@ -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