Explain the sigprocmask in pthread_exit.

Also remove the SIGSEGV special case, which was probably because
hand-written __exit_with_stack_teardown stubs used to try to cause
SIGSEGV if the exit system call returned (which it never does, so
that dead code disappeared).

Also move the sigprocmask into the only case where it's necessary ---
the one where we unmap the stack that would be used by a signal
handler.

Change-Id: Ie40d20c1ae2f5e7125131b6b492cba7a2c6d08e9

diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c
index d2f9254..aa300e9 100644 (file)
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -83,25 +83,20 @@ void __pthread_cleanup_pop( __pthread_cleanup_t*  c, int  execute )
         c->__cleanup_routine(c->__cleanup_arg);
 }
 
-void pthread_exit(void * retval)
-{
-    pthread_internal_t*  thread     = __get_thread();
-    void*                stack_base = thread->attr.stack_base;
-    size_t               stack_size = thread->attr.stack_size;
-    int                  user_stack = (thread->attr.flags & PTHREAD_ATTR_FLAG_USER_STACK) != 0;
-    sigset_t mask;
+void pthread_exit(void* retval) {
+    pthread_internal_t* thread = __get_thread();
 
-    // call the cleanup handlers first
+    // Call the cleanup handlers first.
     while (thread->cleanup_stack) {
-        __pthread_cleanup_t*  c = thread->cleanup_stack;
-        thread->cleanup_stack   = c->__cleanup_prev;
+        __pthread_cleanup_t* c = thread->cleanup_stack;
+        thread->cleanup_stack = c->__cleanup_prev;
         c->__cleanup_routine(c->__cleanup_arg);
     }
 
-    // call the TLS destructors, it is important to do that before removing this
-    // thread from the global list. this will ensure that if someone else deletes
+    // Call the TLS destructors. It is important to do that before removing this
+    // thread from the global list. This will ensure that if someone else deletes
     // a TLS key, the corresponding value will be set to NULL in this thread's TLS
-    // space (see pthread_key_delete)
+    // space (see pthread_key_delete).
     pthread_key_clean_all();
 
     if (thread->alternate_signal_stack != NULL) {
@@ -116,42 +111,49 @@ void pthread_exit(void * retval)
       thread->alternate_signal_stack = NULL;
     }
 
-    // if the thread is detached, destroy the pthread_internal_t
-    // otherwise, keep it in memory and signal any joiners.
+    // Keep track of what we need to know about the stack before we lose the pthread_internal_t.
+    void* stack_base = thread->attr.stack_base;
+    size_t stack_size = thread->attr.stack_size;
+    bool user_allocated_stack = ((thread->attr.flags & PTHREAD_ATTR_FLAG_USER_ALLOCATED_STACK) != 0);
+
+    // If the thread is detached, destroy the pthread_internal_t,
+    // otherwise keep it in memory and signal any joiners.
     pthread_mutex_lock(&gThreadListLock);
     if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) {
         _pthread_internal_remove_locked(thread);
     } else {
-       /* make sure that the thread struct doesn't have stale pointers to a stack that
-        * will be unmapped after the exit call below.
-        */
-        if (!user_stack) {
+        // Make sure that the thread struct doesn't have stale pointers to a stack that
+        // will be unmapped after the exit call below.
+        if (!user_allocated_stack) {
             thread->attr.stack_base = NULL;
             thread->attr.stack_size = 0;
             thread->tls = NULL;
         }
 
-       /* Indicate that the thread has exited for joining threads. */
+        // Indicate that the thread has exited for joining threads.
         thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
         thread->return_value = retval;
 
-       /* Signal the joining thread if present. */
+        // Signal the joining thread if present.
         if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
             pthread_cond_signal(&thread->join_cond);
         }
     }
     pthread_mutex_unlock(&gThreadListLock);
 
-    sigfillset(&mask);
-    sigdelset(&mask, SIGSEGV);
-    sigprocmask(SIG_SETMASK, &mask, NULL);
-
-    if (user_stack) {
+    if (user_allocated_stack) {
         // Cleaning up this thread's stack is the creator's responsibility, not ours.
         __exit(0);
     } else {
         // We need to munmap the stack we're running on before calling exit.
         // That's not something we can do in C.
+
+        // We don't want to take a signal after we've unmapped the stack.
+        // That's one last thing we can handle in C.
+        sigset_t mask;
+        sigfillset(&mask);
+        sigprocmask(SIG_SETMASK, &mask, NULL);
+
         _exit_with_stack_teardown(stack_base, stack_size, 0);
     }
 }

diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index d03b826..f0ee222 100644 (file)
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -175,7 +175,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr,
     }
   } else {
     // The caller did provide a stack, so remember we're not supposed to free it.
-    thread->attr.flags |= PTHREAD_ATTR_FLAG_USER_STACK;
+    thread->attr.flags |= PTHREAD_ATTR_FLAG_USER_ALLOCATED_STACK;
   }
 
   // Make room for the TLS area.
@@ -202,7 +202,7 @@ int pthread_create(pthread_t* thread_out, pthread_attr_t const* attr,
   int tid = __pthread_clone(start_routine, child_stack, flags, arg);
   if (tid < 0) {
     int clone_errno = errno;
-    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_USER_STACK) == 0) {
+    if ((thread->attr.flags & PTHREAD_ATTR_FLAG_USER_ALLOCATED_STACK) == 0) {
       munmap(thread->attr.stack_base, thread->attr.stack_size);
     }
     free(thread);

diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 6b009d4..ce8b410 100644 (file)
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -66,16 +66,16 @@ __LIBC_HIDDEN__ void pthread_key_clean_all(void);
 __LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread);
 
 /* Has the thread been detached by a pthread_join or pthread_detach call? */
-#define PTHREAD_ATTR_FLAG_DETACHED      0x00000001
+#define PTHREAD_ATTR_FLAG_DETACHED 0x00000001
 
 /* Was the thread's stack allocated by the user rather than by us? */
-#define PTHREAD_ATTR_FLAG_USER_STACK    0x00000002
+#define PTHREAD_ATTR_FLAG_USER_ALLOCATED_STACK 0x00000002
 
 /* Has the thread been joined by another thread? */
-#define PTHREAD_ATTR_FLAG_JOINED        0x00000004
+#define PTHREAD_ATTR_FLAG_JOINED 0x00000004
 
 /* Has the thread already exited but not been joined? */
-#define PTHREAD_ATTR_FLAG_ZOMBIE        0x00000008
+#define PTHREAD_ATTR_FLAG_ZOMBIE 0x00000008
 
 #define PTHREAD_INTERNAL_FLAG_THREAD_INIT_FAILED 1