From 4f251bee5d51228217c1bf4dfc9219f3058bd3ed Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Thu, 1 Nov 2012 16:33:29 -0700 Subject: [PATCH] Don't corrupt the thread list if the main thread exits. ...and don't pass a non-heap pointer to free(3), either. This patch replaces the "node** prev" with the clearer "node* prev" style and fixes the null pointer dereference in the old code. That's not sufficient to fix the reporter's bug, though. The pthread_internal_t* for the main thread isn't heap-allocated --- __libc_init_tls causes a pointer to a statically-allocated pthread_internal_t to be added to the thread list. Bug: http://code.google.com/p/android/issues/detail?id=37410 Change-Id: I112b7f22782fc789d58f9c783f7b323bda8fb8b7 --- libc/bionic/libc_init_common.c | 121 ++++++++++++++++----------------- libc/bionic/pthread.c | 66 +++++++++--------- libc/bionic/pthread_internal.h | 3 +- tests/pthread_test.cpp | 17 +++++ 4 files changed, 109 insertions(+), 98 deletions(-) diff --git a/libc/bionic/libc_init_common.c b/libc/bionic/libc_init_common.c index 6508c0b18..fb164f447 100644 --- a/libc/bionic/libc_init_common.c +++ b/libc/bionic/libc_init_common.c @@ -40,10 +40,10 @@ #include extern unsigned __get_sp(void); -extern pid_t gettid(void); +extern pid_t gettid(void); char* __progname; -char **environ; +char** environ; /* from asm/page.h */ unsigned int __page_size = PAGE_SIZE; @@ -60,48 +60,42 @@ int __system_properties_init(void); * stores the pointer in TLS, but does not add it to pthread's gThreadList. This * has to be done later from libc itself (see __libc_init_common). * - * This function also stores elfdata argument in a specific TLS slot to be later + * This function also stores the elf_data argument in a specific TLS slot to be later * picked up by the libc constructor. */ -void __libc_init_tls(unsigned** elfdata) -{ - pthread_attr_t thread_attr; - static pthread_internal_t thread; - static void* tls_area[BIONIC_TLS_SLOTS]; +void __libc_init_tls(unsigned** elf_data) { + unsigned stack_top = (__get_sp() & ~(PAGE_SIZE - 1)) + PAGE_SIZE; + unsigned stack_size = 128 * 1024; + unsigned stack_bottom = stack_top - stack_size; - /* setup pthread runtime and main thread descriptor */ - unsigned stacktop = (__get_sp() & ~(PAGE_SIZE - 1)) + PAGE_SIZE; - unsigned stacksize = 128 * 1024; - unsigned stackbottom = stacktop - stacksize; + pthread_attr_t thread_attr; + pthread_attr_init(&thread_attr); + pthread_attr_setstack(&thread_attr, (void*) stack_bottom, stack_size); - pthread_attr_init(&thread_attr); - pthread_attr_setstack(&thread_attr, (void*)stackbottom, stacksize); - _init_thread(&thread, gettid(), &thread_attr, (void*)stackbottom, false); - __init_tls(tls_area, &thread); + static pthread_internal_t thread; + _init_thread(&thread, gettid(), &thread_attr, (void*) stack_bottom, false); - tls_area[TLS_SLOT_BIONIC_PREINIT] = elfdata; + static void* tls_area[BIONIC_TLS_SLOTS]; + __init_tls(tls_area, &thread); + tls_area[TLS_SLOT_BIONIC_PREINIT] = elf_data; } -void __libc_init_common(uintptr_t *elfdata) -{ - int argc = *elfdata; - char** argv = (char**)(elfdata + 1); - char** envp = argv + argc + 1; +void __libc_init_common(uintptr_t* elf_data) { + int argc = *elf_data; + char** argv = (char**) (elf_data + 1); + char** envp = argv + argc + 1; - /* get the initial thread from TLS and add it to gThreadList */ - _pthread_internal_add(__get_thread()); + // Get the main thread from TLS and add it to the thread list. + pthread_internal_t* main_thread = __get_thread(); + main_thread->allocated_on_heap = false; + _pthread_internal_add(main_thread); - /* clear errno */ - errno = 0; + // Set various globals. + errno = 0; + __progname = argv[0] ? argv[0] : ""; + environ = envp; - /* set program name */ - __progname = argv[0] ? argv[0] : ""; - - /* setup environment pointer */ - environ = envp; - - /* setup system properties - requires environment */ - __system_properties_init(); + __system_properties_init(); // Requires 'environ'. } /* This function will be called during normal program termination @@ -111,39 +105,42 @@ void __libc_init_common(uintptr_t *elfdata) * 'fini_array' points to a list of function addresses. The first * entry in the list has value -1, the last one has value 0. */ -void __libc_fini(void* array) -{ - int count; - void** fini_array = array; - const size_t minus1 = ~(size_t)0; /* ensure proper sign extension */ +void __libc_fini(void* array) { + void** fini_array = array; + const size_t minus1 = ~(size_t)0; /* ensure proper sign extension */ - /* Sanity check - first entry must be -1 */ - if (array == NULL || (size_t)fini_array[0] != minus1) { - return; + /* Sanity check - first entry must be -1 */ + if (array == NULL || (size_t)fini_array[0] != minus1) { + return; + } + + /* skip over it */ + fini_array += 1; + + /* Count the number of destructors. */ + int count = 0; + while (fini_array[count] != NULL) { + ++count; + } + + /* Now call each destructor in reverse order. */ + while (count > 0) { + void (*func)() = (void (*)) fini_array[--count]; + + /* Sanity check, any -1 in the list is ignored */ + if ((size_t)func == minus1) { + continue; } - /* skip over it */ - fini_array += 1; - - /* Count the number of destructors. */ - for (count = 0; fini_array[count] != NULL; count++); - - /* Now call each destructor in reverse order. */ - while (count > 0) { - void (*func)() = (void (*)) fini_array[--count]; - - /* Sanity check, any -1 in the list is ignored */ - if ((size_t)func == minus1) - continue; - - func(); - } + func(); + } #ifndef LIBC_STATIC - { - extern void __libc_postfini(void) __attribute__((weak)); - if (__libc_postfini) - __libc_postfini(); + { + extern void __libc_postfini(void) __attribute__((weak)); + if (__libc_postfini) { + __libc_postfini(); } + } #endif } diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c index 791a772b0..d8d0d0541 100644 --- a/libc/bionic/pthread.c +++ b/libc/bionic/pthread.c @@ -105,44 +105,41 @@ static pthread_internal_t* gThreadList = NULL; static pthread_mutex_t gThreadListLock = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t gDebuggerNotificationLock = PTHREAD_MUTEX_INITIALIZER; - -static void -_pthread_internal_free(pthread_internal_t* thread) -{ - if (thread != NULL) { - free(thread); - } -} - - -static void -_pthread_internal_remove_locked( pthread_internal_t* thread ) -{ +static void _pthread_internal_remove_locked(pthread_internal_t* thread) { + if (thread->next != NULL) { thread->next->prev = thread->prev; - thread->prev[0] = thread->next; + } + if (thread->prev != NULL) { + thread->prev->next = thread->next; + } else { + gThreadList = thread->next; + } + + // The main thread is not heap-allocated. See __libc_init_tls for the declaration, + // and __libc_init_common for the point where it's added to the thread list. + if (thread->allocated_on_heap) { + free(thread); + } } -static void -_pthread_internal_remove( pthread_internal_t* thread ) -{ - pthread_mutex_lock(&gThreadListLock); - _pthread_internal_remove_locked(thread); - pthread_mutex_unlock(&gThreadListLock); +static void _pthread_internal_remove(pthread_internal_t* thread) { + pthread_mutex_lock(&gThreadListLock); + _pthread_internal_remove_locked(thread); + pthread_mutex_unlock(&gThreadListLock); } -__LIBC_ABI_PRIVATE__ void -_pthread_internal_add(pthread_internal_t* thread) -{ - pthread_mutex_lock(&gThreadListLock); +__LIBC_ABI_PRIVATE__ void _pthread_internal_add(pthread_internal_t* thread) { + pthread_mutex_lock(&gThreadListLock); - thread->prev = &gThreadList; - thread->next = *(thread->prev); - if (thread->next != NULL) { - thread->next->prev = &thread->next; - } - *(thread->prev) = thread; + // We insert at the head. + thread->next = gThreadList; + thread->prev = NULL; + if (thread->next != NULL) { + thread->next->prev = thread; + } + gThreadList = thread; - pthread_mutex_unlock(&gThreadListLock); + pthread_mutex_unlock(&gThreadListLock); } __LIBC_ABI_PRIVATE__ pthread_internal_t* @@ -312,6 +309,7 @@ int pthread_create(pthread_t *thread_out, pthread_attr_t const * attr, if (thread == NULL) { return ENOMEM; } + thread->allocated_on_heap = true; if (attr == NULL) { attr = &gDefaultPthreadAttr; @@ -323,7 +321,7 @@ int pthread_create(pthread_t *thread_out, pthread_attr_t const * attr, if (stack == NULL) { stack = mkstack(stack_size, attr->guard_size); if (stack == NULL) { - _pthread_internal_free(thread); + free(thread); return ENOMEM; } } @@ -353,7 +351,7 @@ int pthread_create(pthread_t *thread_out, pthread_attr_t const * attr, if (stack != attr->stack_base) { munmap(stack, stack_size); } - _pthread_internal_free(thread); + free(thread); errno = old_errno; return clone_errno; } @@ -585,7 +583,6 @@ void pthread_exit(void * retval) // otherwise, keep it in memory and signal any joiners. if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) { _pthread_internal_remove(thread); - _pthread_internal_free(thread); } else { pthread_mutex_lock(&gThreadListLock); @@ -677,7 +674,6 @@ FoundIt: */ if (count <= 0) { _pthread_internal_remove_locked(thread); - _pthread_internal_free(thread); } pthread_mutex_unlock(&gThreadListLock); return 0; diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h index a6b44c718..bc682917d 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -36,9 +36,10 @@ __BEGIN_DECLS typedef struct pthread_internal_t { struct pthread_internal_t* next; - struct pthread_internal_t** prev; + struct pthread_internal_t* prev; pthread_attr_t attr; pid_t kernel_id; + bool allocated_on_heap; pthread_cond_t join_cond; int join_count; void* return_value; diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index e400b84ab..da945b4df 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -109,3 +109,20 @@ TEST(pthread, pthread_join_self) { void* result; ASSERT_EQ(EDEADLK, pthread_join(pthread_self(), &result)); } + +#if __BIONIC__ // For some reason, gtest on bionic can cope with this but gtest on glibc can't. + +static void TestBug37410() { + pthread_t t1; + ASSERT_EQ(0, pthread_create(&t1, NULL, JoinFn, reinterpret_cast(pthread_self()))); + pthread_exit(NULL); +} + +// We have to say "DeathTest" here so gtest knows to run this test (which exits) +// in its own process. +TEST(pthread_DeathTest, pthread_bug_37410) { + // http://code.google.com/p/android/issues/detail?id=37410 + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + EXPECT_EXIT(TestBug37410(), ::testing::ExitedWithCode(0), ""); +} +#endif