Re: linuxthread race condition

[Andreas Schwab <schwab at suse dot de>]

Ulrich Drepper <drepper@redhat.com> writes:

|> Andreas Schwab <schwab@suse.de> writes:
|> 
|> > +static pthread_descr thread_self_stack(void)
|> > +{
|> > +  char *sp = CURRENT_STACK_FRAME;
|> > +  pthread_handle h;
|> > +
|> > +  if (sp >= __pthread_initial_thread_bos)
|> > +    return &__pthread_initial_thread;
|> > +  else if (sp >= __pthread_manager_thread_bos
|> > +	   && sp < __pthread_manager_thread_tos)
|> > +    return &__pthread_manager_thread;
|> > +  else
|> > +    {
|> > +      h = __pthread_handles + 2;
|> > +      while (! (sp <= (char *) h->h_descr && sp >= h->h_bottom))
|> > +	h++;
|> > +      return h->h_descr;
|> > +    }
|> > +}
|> > +
|> >  #endif
|> 
|> This is overkill.  You use this function to check whether the thread
|> is the manager via the stack pointer.  But the function contains a lot
|> of tests and returns useful values for all other cases as well.  All
|> the tests except the first else are unnecessary.

No, they are not.  We need the real thread descriptor for the rest of the
signal handler.  In fact, my patch is still not enough, we must call
INIT_THREAD_SELF so that THREAD_GETMEM is using the correct thread
pointer.

Andreas.

2001-11-30  Andreas Schwab  <schwab@suse.de>

	* pthread.c (pthread_handle_sigcancel) [THREAD_SELF]: Double check
	that self is the manager thread, and initialize the thread
	register if not.
	(thread_self_stack) [THREAD_SELF]: New function to find self via
	stack pointer.
	* manager.c (pthread_handle_create): Don't block cancel signal any
	more.

--- manager.c.~1.76.~	Thu Nov 29 10:40:10 2001
+++ manager.c	Thu Nov 29 20:37:59 2001
@@ -534,7 +534,6 @@
   size_t guardsize = 0;
   int pagesize = __getpagesize();
   int saved_errno;
-  sigset_t newmask, oldmask;
 
   /* First check whether we have to change the policy and if yes, whether
      we can  do this.  Normally this should be done by examining the
@@ -620,11 +619,6 @@
       if ((mask & (__pthread_threads_events.event_bits[idx]
 		   | event_maskp->event_bits[idx])) != 0)
 	{
-	  /* Block cancel signal in the child until it is fully
-	     initialized.  */
-	  sigemptyset(&newmask);
-	  sigaddset(&newmask, __pthread_sig_cancel);
-	  sigprocmask(SIG_BLOCK, &newmask, &oldmask);
 	  /* Lock the mutex the child will use now so that it will stop.  */
 	  __pthread_lock(new_thread->p_lock, NULL);
 
@@ -653,7 +647,6 @@
 			__pthread_sig_cancel, new_thread);
 #endif
 	  saved_errno = errno;
-	  sigprocmask(SIG_SETMASK, &oldmask, NULL);
 	  if (pid != -1)
 	    {
 	      /* Now fill in the information about the new thread in
@@ -679,11 +672,6 @@
     }
   if (pid == 0)
     {
-      /* Block cancel signal in the child until it is fully
-	 initialized.  */
-      sigemptyset(&newmask);
-      sigaddset(&newmask, __pthread_sig_cancel);
-      sigprocmask(SIG_BLOCK, &newmask, &oldmask);
 #ifdef NEED_SEPARATE_REGISTER_STACK
       pid = __clone2(pthread_start_thread,
 		     (void **)new_thread_bottom,
@@ -700,7 +688,6 @@
 		    __pthread_sig_cancel, new_thread);
 #endif /* !NEED_SEPARATE_REGISTER_STACK */
       saved_errno = errno;
-      sigprocmask(SIG_SETMASK, &oldmask, NULL);
     }
   /* Check if cloning succeeded */
   if (pid == -1) {
--- pthread.c.~1.88.~	Thu Nov 29 10:40:10 2001
+++ pthread.c	Fri Nov 30 10:38:26 2001
@@ -709,7 +709,7 @@
 
 #ifndef THREAD_SELF
 
-pthread_descr __pthread_find_self()
+pthread_descr __pthread_find_self(void)
 {
   char * sp = CURRENT_STACK_FRAME;
   pthread_handle h;
@@ -721,6 +721,27 @@
   return h->h_descr;
 }
 
+#else
+
+static pthread_descr thread_self_stack(void)
+{
+  char *sp = CURRENT_STACK_FRAME;
+  pthread_handle h;
+
+  if (sp >= __pthread_initial_thread_bos)
+    return &__pthread_initial_thread;
+  else if (sp >= __pthread_manager_thread_bos
+	   && sp < __pthread_manager_thread_tos)
+    return &__pthread_manager_thread;
+  else
+    {
+      h = __pthread_handles + 2;
+      while (! (sp <= (char *) h->h_descr && sp >= h->h_bottom))
+	h++;
+      return h->h_descr;
+    }
+}
+
 #endif
 
 /* Thread scheduling */
@@ -769,7 +790,7 @@
   return 0;
 }
 
-int __pthread_yield ()
+int __pthread_yield (void)
 {
   /* For now this is equivalent with the POSIX call.  */
   return sched_yield ();
@@ -841,8 +862,26 @@
 
   if (self == &__pthread_manager_thread)
     {
+#ifdef THREAD_SELF
+      /* A new thread might get a cancel signal before it is fully
+	 initialized, so that the thread register might still point to the
+	 manager thread.  Double check that this is really the manager
+	 thread.  */
+      pthread_descr real_self = thread_self_stack();
+      if (real_self == &__pthread_manager_thread)
+	{
+	  __pthread_manager_sighandler(sig);
+	  return;
+	}
+      /* Oops, thread_self() isn't working yet..  */
+      self = real_self;
+# ifdef INIT_THREAD_SELF
+      INIT_THREAD_SELF(self, self->p_nr);
+# endif
+#else
       __pthread_manager_sighandler(sig);
       return;
+#endif
     }
   if (__builtin_expect (__pthread_exit_requested, 0)) {
     /* Main thread should accumulate times for thread manager and its
@@ -884,7 +923,7 @@
    Notice that we can't free the stack segments, as the forked thread
    may hold pointers into them. */
 
-void __pthread_reset_main_thread()
+void __pthread_reset_main_thread(void)
 {
   pthread_descr self = thread_self();
   struct rlimit limit;

-- 
Andreas Schwab                                  "And now for something
Andreas.Schwab@suse.de				completely different."
SuSE Labs, SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5