Fix races in setXid implementation.

3 files changed, 123 insertions, 24 deletions

diff --git a/nptl/ChangeLog b/nptl/ChangeLog
index d211635..f61cb5c 100644
--- a/nptl/ChangeLog
+++ b/nptl/ChangeLog
@@ -1,3 +1,11 @@
+2009-10-30  Ulrich Drepper  <drepper@redhat.com>
+
+	[BZ #3270]
+	* allocatestack.c (__nptl_setxid): Perform the operation in multiple
+	steps to avoid races with creation and terminations.
+	* nptl-init.c (sighandler_setxid): Adjust.
+	Patch by Daniel Jacobowitz.
+
 2009-09-07  Andreas Schwab  <schwab@redhat.com>
 
 	* sysdeps/pthread/bits/libc-lock.h (BP_SYM): Remove space before paren.
diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
index 67ea0c6..3c3585f 100644
--- a/nptl/allocatestack.c
+++ b/nptl/allocatestack.c
@@ -965,22 +965,53 @@ __find_thread_by_id (pid_t tid)
 
 static void
 internal_function
-setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
+setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
 {
-  if (! IS_DETACHED (t))
+  int ch;
+
+  /* Don't let the thread exit before the setxid handler runs.  */
+  t->setxid_futex = 0;
+
+  do
     {
-      int ch;
-      do
-	{
-	  ch = t->cancelhandling;
+      ch = t->cancelhandling;
 
-	  /* If the thread is exiting right now, ignore it.  */
-	  if ((ch & EXITING_BITMASK) != 0)
-	    return;
-	}
-      while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
-						   ch | SETXID_BITMASK, ch));
+      /* If the thread is exiting right now, ignore it.  */
+      if ((ch & EXITING_BITMASK) != 0)
+	return;
     }
+  while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
+					       ch | SETXID_BITMASK, ch));
+}
+
+
+static void
+internal_function
+setxid_unmark_thread (struct xid_command *cmdp, struct pthread *t)
+{
+  int ch;
+
+  do
+    {
+      ch = t->cancelhandling;
+      if ((ch & SETXID_BITMASK) == 0)
+	return;
+    }
+  while (atomic_compare_and_exchange_bool_acq (&t->cancelhandling,
+					       ch & ~SETXID_BITMASK, ch));
+
+  /* Release the futex just in case.  */
+  t->setxid_futex = 1;
+  lll_futex_wake (&t->setxid_futex, 1, LLL_PRIVATE);
+}
+
+
+static int
+internal_function
+setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
+{
+  if ((t->cancelhandling & SETXID_BITMASK) == 0)
+    return 0;
 
   int val;
   INTERNAL_SYSCALL_DECL (err);
@@ -997,8 +1028,14 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
     val = INTERNAL_SYSCALL (tkill, err, 2, t->tid, SIGSETXID);
 #endif
 
+  /* If this failed, it must have had not started yet or else exited.  */
   if (!INTERNAL_SYSCALL_ERROR_P (val, err))
-    atomic_increment (&cmdp->cntr);
+    {
+      atomic_increment (&cmdp->cntr);
+      return 1;
+    }
+  else
+    return 0;
 }
 
 
@@ -1006,6 +1043,7 @@ int
 attribute_hidden
 __nptl_setxid (struct xid_command *cmdp)
 {
+  int signalled;
   int result;
   lll_lock (stack_cache_lock, LLL_PRIVATE);
 
@@ -1022,7 +1060,7 @@ __nptl_setxid (struct xid_command *cmdp)
       if (t == self)
 	continue;
 
-      setxid_signal_thread (cmdp, t);
+      setxid_mark_thread (cmdp, t);
     }
 
   /* Now the list with threads using user-allocated stacks.  */
@@ -1032,14 +1070,61 @@ __nptl_setxid (struct xid_command *cmdp)
       if (t == self)
 	continue;
 
-      setxid_signal_thread (cmdp, t);
+      setxid_mark_thread (cmdp, t);
     }
 
-  int cur = cmdp->cntr;
-  while (cur != 0)
+  /* Iterate until we don't succeed in signalling anyone.  That means
+     we have gotten all running threads, and their children will be
+     automatically correct once started.  */
+  do
     {
-      lll_futex_wait (&cmdp->cntr, cur, LLL_PRIVATE);
-      cur = cmdp->cntr;
+      signalled = 0;
+
+      list_for_each (runp, &stack_used)
+	{
+	  struct pthread *t = list_entry (runp, struct pthread, list);
+	  if (t == self)
+	    continue;
+
+	  signalled += setxid_signal_thread (cmdp, t);
+	}
+
+      list_for_each (runp, &__stack_user)
+	{
+	  struct pthread *t = list_entry (runp, struct pthread, list);
+	  if (t == self)
+	    continue;
+
+	  signalled += setxid_signal_thread (cmdp, t);
+	}
+
+      int cur = cmdp->cntr;
+      while (cur != 0)
+	{
+	  lll_futex_wait (&cmdp->cntr, cur, LLL_PRIVATE);
+	  cur = cmdp->cntr;
+	}
+    }
+  while (signalled != 0);
+
+  /* Clean up flags, so that no thread blocks during exit waiting
+     for a signal which will never come.  */
+  list_for_each (runp, &stack_used)
+    {
+      struct pthread *t = list_entry (runp, struct pthread, list);
+      if (t == self)
+	continue;
+
+      setxid_unmark_thread (cmdp, t);
+    }
+
+  list_for_each (runp, &__stack_user)
+    {
+      struct pthread *t = list_entry (runp, struct pthread, list);
+      if (t == self)
+	continue;
+
+      setxid_unmark_thread (cmdp, t);
     }
 
   /* This must be last, otherwise the current thread might not have
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 5e9c250..851bab2 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -240,17 +240,23 @@ sighandler_setxid (int sig, siginfo_t *si, void *ctx)
   INTERNAL_SYSCALL_NCS (__xidcmd->syscall_no, err, 3, __xidcmd->id[0],
 			__xidcmd->id[1], __xidcmd->id[2]);
 
-  if (atomic_decrement_val (&__xidcmd->cntr) == 0)
-    lll_futex_wake (&__xidcmd->cntr, 1, LLL_PRIVATE);
-
   /* Reset the SETXID flag.  */
   struct pthread *self = THREAD_SELF;
-  int flags = THREAD_GETMEM (self, cancelhandling);
-  THREAD_SETMEM (self, cancelhandling, flags & ~SETXID_BITMASK);
+  int flags, newval;
+  do
+    {
+      flags = THREAD_GETMEM (self, cancelhandling);
+      newval = THREAD_ATOMIC_CMPXCHG_VAL (self, cancelhandling,
+					  flags & ~SETXID_BITMASK, flags);
+    }
+  while (flags != newval);
 
   /* And release the futex.  */
   self->setxid_futex = 1;
   lll_futex_wake (&self->setxid_futex, 1, LLL_PRIVATE);
+
+  if (atomic_decrement_val (&__xidcmd->cntr) == 0)
+    lll_futex_wake (&__xidcmd->cntr, 1, LLL_PRIVATE);
 }