From 2b6e43e00ece68b3aba26d8f95f07cd9d9294ab4 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Tue, 29 Oct 2013 16:11:06 -0700 Subject: [PATCH] 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 --- libc/bionic/pthread.c | 54 ++++++++++++++++++---------------- libc/bionic/pthread_create.cpp | 4 +-- libc/bionic/pthread_internal.h | 8 ++--- 3 files changed, 34 insertions(+), 32 deletions(-) diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c index d2f9254c2..aa300e9e9 100644 --- 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 d03b8269c..f0ee22242 100644 --- 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 6b009d449..ce8b410bd 100644 --- 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