From bfeab1bbe7e8d0c08b7e3f46aedab64e3b2bf706 Mon Sep 17 00:00:00 2001 From: Elliott Hughes Date: Wed, 5 Sep 2012 17:47:37 -0700 Subject: [PATCH] Don't corrupt the thread list in static executables. Several previous changes conspired to make a mess of the thread list in static binaries. This was most obvious when trying to call pthread_key_delete(3) on the main thread. Bug: http://code.google.com/p/android/issues/detail?id=36893 Change-Id: I2a2f553114d8fb40533c481252b410c10656da2e --- libc/bionic/libc_init_common.c | 2 +- libc/bionic/libc_init_static.c | 3 --- libc/bionic/pthread.c | 36 ++++++++++++++++++++-------------- libc/bionic/pthread_internal.h | 10 ++++++---- tests/Android.mk | 14 ++++++++++++- tests/pthread_test.cpp | 28 ++++++++++++++++++++++++++ 6 files changed, 69 insertions(+), 24 deletions(-) create mode 100644 tests/pthread_test.cpp diff --git a/libc/bionic/libc_init_common.c b/libc/bionic/libc_init_common.c index 4ce4db629..6508c0b18 100644 --- a/libc/bionic/libc_init_common.c +++ b/libc/bionic/libc_init_common.c @@ -76,7 +76,7 @@ void __libc_init_tls(unsigned** elfdata) pthread_attr_init(&thread_attr); pthread_attr_setstack(&thread_attr, (void*)stackbottom, stacksize); - _init_thread(&thread, gettid(), &thread_attr, (void*)stackbottom); + _init_thread(&thread, gettid(), &thread_attr, (void*)stackbottom, false); __init_tls(tls_area, &thread); tls_area[TLS_SLOT_BIONIC_PREINIT] = elfdata; diff --git a/libc/bionic/libc_init_static.c b/libc/bionic/libc_init_static.c index 97156e8d5..a73bb711b 100644 --- a/libc/bionic/libc_init_static.c +++ b/libc/bionic/libc_init_static.c @@ -67,9 +67,6 @@ __noreturn void __libc_init(uintptr_t *elfdata, __libc_init_tls(NULL); - /* get the initial thread from TLS and add it to gThreadList */ - _pthread_internal_add(__get_thread()); - /* Initialize the C runtime environment */ __libc_init_common(elfdata); diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c index 40a09ba6e..14d2fec53 100644 --- a/libc/bionic/pthread.c +++ b/libc/bionic/pthread.c @@ -117,8 +117,8 @@ _pthread_internal_free(pthread_internal_t* thread) static void _pthread_internal_remove_locked( pthread_internal_t* thread ) { - thread->next->pref = thread->pref; - thread->pref[0] = thread->next; + thread->next->prev = thread->prev; + thread->prev[0] = thread->next; } static void @@ -130,14 +130,17 @@ _pthread_internal_remove( pthread_internal_t* thread ) } __LIBC_ABI_PRIVATE__ void -_pthread_internal_add( pthread_internal_t* thread ) +_pthread_internal_add(pthread_internal_t* thread) { pthread_mutex_lock(&gThreadListLock); - thread->pref = &gThreadList; - thread->next = thread->pref[0]; - if (thread->next) - thread->next->pref = &thread->next; - thread->pref[0] = thread; + + thread->prev = &gThreadList; + thread->next = *(thread->prev); + if (thread->next != NULL) { + thread->next->prev = &thread->next; + } + *(thread->prev) = thread; + pthread_mutex_unlock(&gThreadListLock); } @@ -203,7 +206,8 @@ void __thread_entry(int (*func)(void*), void *arg, void **tls) } __LIBC_ABI_PRIVATE__ -int _init_thread(pthread_internal_t * thread, pid_t kernel_id, pthread_attr_t * attr, void * stack_base) +int _init_thread(pthread_internal_t* thread, pid_t kernel_id, pthread_attr_t* attr, + void* stack_base, bool add_to_thread_list) { int error = 0; @@ -231,10 +235,12 @@ int _init_thread(pthread_internal_t * thread, pid_t kernel_id, pthread_attr_t * pthread_cond_init(&thread->join_cond, NULL); thread->join_count = 0; - thread->cleanup_stack = NULL; - _pthread_internal_add(thread); + if (add_to_thread_list) { + _pthread_internal_add(thread); + } + return error; } @@ -348,7 +354,7 @@ int pthread_create(pthread_t *thread_out, pthread_attr_t const * attr, return clone_errno; } - int init_errno = _init_thread(thread, tid, (pthread_attr_t*) attr, stack); + int init_errno = _init_thread(thread, tid, (pthread_attr_t*) attr, stack, true); if (init_errno != 0) { // Mark the thread detached and let its __thread_entry run to // completion. (It'll just exit immediately, cleaning up its resources.) @@ -1919,8 +1925,8 @@ int pthread_key_create(pthread_key_t *key, void (*destructor_function)(void *)) /* This deletes a pthread_key_t. note that the standard mandates that this does * not call the destructor of non-NULL key values. Instead, it is the - * responsability of the caller to properly dispose of the corresponding data - * and resources, using any mean it finds suitable. + * responsibility of the caller to properly dispose of the corresponding data + * and resources, using any means it finds suitable. * * On the other hand, this function will clear the corresponding key data * values in all known threads. this prevents later (invalid) calls to @@ -2184,7 +2190,7 @@ int pthread_once( pthread_once_t* once_control, void (*init_routine)(void) ) for (;;) { /* Try to atomically set the INITIALIZING flag. * This requires a cmpxchg loop, and we may need - * to exit prematurely if we detect that + * to exit prematurely if we detect that * COMPLETED is now set. */ int32_t oldval, newval; diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h index 2bd110c60..58a809a26 100644 --- a/libc/bionic/pthread_internal.h +++ b/libc/bionic/pthread_internal.h @@ -29,13 +29,14 @@ #define _PTHREAD_INTERNAL_H_ #include +#include __BEGIN_DECLS typedef struct pthread_internal_t { struct pthread_internal_t* next; - struct pthread_internal_t** pref; + struct pthread_internal_t** prev; pthread_attr_t attr; pid_t kernel_id; pthread_cond_t join_cond; @@ -46,7 +47,8 @@ typedef struct pthread_internal_t void** tls; /* thread-local storage area */ } pthread_internal_t; -extern int _init_thread(pthread_internal_t * thread, pid_t kernel_id, pthread_attr_t * attr, void * stack_base); +int _init_thread(pthread_internal_t* thread, pid_t kernel_id, pthread_attr_t* attr, + void* stack_base, bool add_to_thread_list); void _pthread_internal_add( pthread_internal_t* thread ); pthread_internal_t* __get_thread(void); @@ -100,9 +102,9 @@ static __inline__ int timespec_cmp0( const struct timespec* a ) return 0; } -extern int __pthread_cond_timedwait(pthread_cond_t*, +extern int __pthread_cond_timedwait(pthread_cond_t*, pthread_mutex_t*, - const struct timespec*, + const struct timespec*, clockid_t); extern int __pthread_cond_timedwait_relative(pthread_cond_t*, diff --git a/tests/Android.mk b/tests/Android.mk index 1219afda7..ae57d9bfb 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -20,9 +20,10 @@ LOCAL_PATH := $(call my-dir) test_src_files = \ getcwd_test.cpp \ + pthread_test.cpp \ regex_test.cpp \ -# Build for the device (with bionic). Run with: +# Build for the device (with bionic's .so). Run with: # adb shell /data/nativetest/bionic-unit-tests/bionic-unit-tests include $(CLEAR_VARS) LOCAL_MODULE := bionic-unit-tests @@ -30,6 +31,16 @@ LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_SRC_FILES := $(test_src_files) include $(BUILD_NATIVE_TEST) +# Build for the device (with bionic's .a). Run with: +# adb shell /data/nativetest/bionic-unit-tests-static/bionic-unit-tests-static +include $(CLEAR_VARS) +LOCAL_FORCE_STATIC_EXECUTABLE := true +LOCAL_STATIC_LIBRARIES += libstlport_static libstdc++ libm libc +LOCAL_MODULE := bionic-unit-tests-static +LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk +LOCAL_SRC_FILES := $(test_src_files) +include $(BUILD_NATIVE_TEST) + # Build for the host (with glibc). # Note that this will build against glibc, so it's not useful for testing # bionic's implementation, but it does let you use glibc as a reference @@ -37,6 +48,7 @@ include $(BUILD_NATIVE_TEST) include $(CLEAR_VARS) LOCAL_MODULE := bionic-unit-tests-glibc LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk +LOCAL_LDFLAGS += -lpthread LOCAL_SRC_FILES := $(test_src_files) include $(BUILD_HOST_NATIVE_TEST) diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp new file mode 100644 index 000000000..04975df2f --- /dev/null +++ b/tests/pthread_test.cpp @@ -0,0 +1,28 @@ +/* + * Copyright (C) 2012 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +TEST(pthread, pthread_key_create) { + pthread_key_t key; + ASSERT_EQ(0, pthread_key_create(&key, NULL)); + ASSERT_EQ(0, pthread_key_delete(key)); + // Can't delete a key that's already been deleted. + ASSERT_EQ(EINVAL, pthread_key_delete(key)); +}