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
diff --git a/libc/bionic/libc_init_common.c b/libc/bionic/libc_init_common.c
index 6508c0b..fb164f4 100644
--- a/libc/bionic/libc_init_common.c
+++ b/libc/bionic/libc_init_common.c
@@ -40,10 +40,10 @@
#include <errno.h>
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 @@
* 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] : "<unknown>";
+ environ = envp;
- /* set program name */
- __progname = argv[0] ? argv[0] : "<unknown>";
-
- /* 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 @@
* '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 791a772..d8d0d05 100644
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -105,44 +105,41 @@
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 @@
if (thread == NULL) {
return ENOMEM;
}
+ thread->allocated_on_heap = true;
if (attr == NULL) {
attr = &gDefaultPthreadAttr;
@@ -323,7 +321,7 @@
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 @@
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 @@
// 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 @@
*/
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 a6b44c7..bc68291 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -36,9 +36,10 @@
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 e400b84..da945b4 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -109,3 +109,20 @@
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<void*>(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