am e3c7b519: Merge "Don\'t corrupt the thread list if the main thread exits."

* commit 'e3c7b5192e65eeb0bd90bf884d3435ed9adfad0e':
  Don't corrupt the thread list if the main thread exits.

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 @@ 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)
+void __libc_init_tls(unsigned** elf_data) {
-{
+  unsigned stack_top = (__get_sp() & ~(PAGE_SIZE - 1)) + PAGE_SIZE;
-    pthread_attr_t             thread_attr;
+  unsigned stack_size = 128 * 1024;
-    static pthread_internal_t  thread;
+  unsigned stack_bottom = stack_top - stack_size;
-    static void*               tls_area[BIONIC_TLS_SLOTS];
 
-    /* setup pthread runtime and main thread descriptor */
+  pthread_attr_t thread_attr;
-    unsigned stacktop = (__get_sp() & ~(PAGE_SIZE - 1)) + PAGE_SIZE;
+  pthread_attr_init(&thread_attr);
-    unsigned stacksize = 128 * 1024;
+  pthread_attr_setstack(&thread_attr, (void*) stack_bottom, stack_size);
-    unsigned stackbottom = stacktop - stacksize;
 
-    pthread_attr_init(&thread_attr);
+  static pthread_internal_t thread;
-    pthread_attr_setstack(&thread_attr, (void*)stackbottom, stacksize);
+  _init_thread(&thread, gettid(), &thread_attr, (void*) stack_bottom, false);
-    _init_thread(&thread, gettid(), &thread_attr, (void*)stackbottom, false);
-    __init_tls(tls_area, &thread);
 
-    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)
+void __libc_init_common(uintptr_t* elf_data) {
-{
+  int argc = *elf_data;
-    int     argc = *elfdata;
+  char** argv = (char**) (elf_data + 1);
-    char**  argv = (char**)(elfdata + 1);
+  char** envp = argv + argc + 1;
-    char**  envp = argv + argc + 1;
 
-    /* get the initial thread from TLS and add it to gThreadList */
+  // Get the main thread from TLS and add it to the thread list.
-    _pthread_internal_add(__get_thread());
+  pthread_internal_t* main_thread = __get_thread();
+  main_thread->allocated_on_heap = false;
+  _pthread_internal_add(main_thread);
 
-    /* clear errno */
+  // Set various globals.
-    errno = 0;
+  errno = 0;
+  __progname = argv[0] ? argv[0] : "<unknown>";
+  environ = envp;
 
-    /* set program name */
+  __system_properties_init(); // Requires 'environ'.
-    __progname = argv[0] ? argv[0] : "<unknown>";
-
-    /* setup environment pointer */
-    environ = envp;
-
-    /* setup system properties - requires environment */
-    __system_properties_init();
 }
 
 /* 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)
+void __libc_fini(void* array) {
-{
+  void** fini_array = array;
-    int count;
+  const size_t minus1 = ~(size_t)0; /* ensure proper sign extension */
-    void** fini_array = array;
-    const size_t  minus1 = ~(size_t)0; /* ensure proper sign extension */
 
-    /* Sanity check - first entry must be -1 */
+  /* Sanity check - first entry must be -1 */
-    if (array == NULL || (size_t)fini_array[0] != minus1) {
+  if (array == NULL || (size_t)fini_array[0] != minus1) {
-        return;
+    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 */
+    func();
-    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();
-    }
 
 #ifndef LIBC_STATIC
-    {
+  {
-        extern void __libc_postfini(void) __attribute__((weak));
+    extern void __libc_postfini(void) __attribute__((weak));
-        if (__libc_postfini)
+    if (__libc_postfini) {
-            __libc_postfini();
+      __libc_postfini();
     }
+  }
 #endif
 }

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_remove_locked(pthread_internal_t* thread) {
-static void
+  if (thread->next != NULL) {
-_pthread_internal_free(pthread_internal_t* thread)
-{
-    if (thread != NULL) {
-        free(thread);
-    }
-}
-
-
-static void
-_pthread_internal_remove_locked( pthread_internal_t*  thread )
-{
     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
+static void _pthread_internal_remove(pthread_internal_t* thread) {
-_pthread_internal_remove( pthread_internal_t*  thread )
+  pthread_mutex_lock(&gThreadListLock);
-{
+  _pthread_internal_remove_locked(thread);
-    pthread_mutex_lock(&gThreadListLock);
+  pthread_mutex_unlock(&gThreadListLock);
-    _pthread_internal_remove_locked(thread);
-    pthread_mutex_unlock(&gThreadListLock);
 }
 
-__LIBC_ABI_PRIVATE__ void
+__LIBC_ABI_PRIVATE__ void _pthread_internal_add(pthread_internal_t* thread) {
-_pthread_internal_add(pthread_internal_t* thread)
+  pthread_mutex_lock(&gThreadListLock);
-{
-    pthread_mutex_lock(&gThreadListLock);
 
-    thread->prev = &gThreadList;
+  // We insert at the head.
-    thread->next = *(thread->prev);
+  thread->next = gThreadList;
-    if (thread->next != NULL) {
+  thread->prev = NULL;
-        thread->next->prev = &thread->next;
+  if (thread->next != NULL) {
-    }
+    thread->next->prev = thread;
-    *(thread->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;

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;

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<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