Fix pthread_join.
Let the kernel keep pthread_internal_t::tid updated, including
across forks and for the main thread. This then lets us fix
pthread_join to only return after the thread has really exited.
Also fix the thread attributes of the main thread so we don't
unmap the main thread's stack (which is really owned by the
dynamic linker and contains things like environment variables),
which fixes crashes when joining with an exited main thread
and also fixes problems reported publicly with accessing environment
variables after the main thread exits (for which I've added a new
unit test).
In passing I also fixed a bug where if the clone(2) inside
pthread_create(3) fails, we'd unmap the child's stack and TLS (which
contains the mutex) and then try to unlock the mutex. Boom! It wasn't
until after I'd uploaded the fix for this that I came across a new
public bug reporting this exact failure.
Bug: 8206355
Bug: 11693195
Bug: https://code.google.com/p/android/issues/detail?id=57421
Bug: https://code.google.com/p/android/issues/detail?id=62392
Change-Id: I2af9cf6e8ae510a67256ad93cad891794ed0580b
diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index 6ed01ff..dde5ed7 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -97,7 +97,6 @@
}
}
- pthread_cond_init(&thread->join_cond, NULL);
thread->cleanup_stack = NULL;
if (add_to_thread_list) {
@@ -215,17 +214,22 @@
// the new thread.
pthread_mutex_t* start_mutex = (pthread_mutex_t*) &thread->tls[TLS_SLOT_START_MUTEX];
pthread_mutex_init(start_mutex, NULL);
- ScopedPthreadMutexLocker start_locker(start_mutex);
+ pthread_mutex_lock(start_mutex);
thread->tls[TLS_SLOT_THREAD_ID] = thread;
thread->start_routine = start_routine;
thread->start_routine_arg = arg;
- int flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM | CLONE_SETTLS;
- int tid = __bionic_clone(flags, child_stack, NULL, thread->tls, NULL, __pthread_start, thread);
- if (tid < 0) {
+ int flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM |
+ CLONE_SETTLS | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID;
+ int rc = __bionic_clone(flags, child_stack, &(thread->tid), thread->tls, &(thread->tid), __pthread_start, thread);
+ if (rc == -1) {
int clone_errno = errno;
+ // We don't have to unlock the mutex at all because clone(2) failed so there's no child waiting to
+ // be unblocked, but we're about to unmap the memory the mutex is stored in, so this serves as a
+ // reminder that you can't rewrite this function to use a ScopedPthreadMutexLocker.
+ pthread_mutex_unlock(start_mutex);
if ((thread->attr.flags & PTHREAD_ATTR_FLAG_USER_ALLOCATED_STACK) == 0) {
munmap(thread->attr.stack_base, thread->attr.stack_size);
}
@@ -234,12 +238,10 @@
return clone_errno;
}
- thread->tid = tid;
-
int init_errno = _init_thread(thread, true);
if (init_errno != 0) {
- // Mark the thread detached and let its __pthread_start run to
- // completion. (It'll just exit immediately, cleaning up its resources.)
+ // Mark the thread detached and let its __pthread_start run to completion.
+ // It'll check this flag and exit immediately, cleaning up its resources.
thread->internal_flags |= PTHREAD_INTERNAL_FLAG_THREAD_INIT_FAILED;
thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
return init_errno;
@@ -251,8 +253,9 @@
_thread_created_hook(thread->tid);
}
- // Publish the pthread_t and let the thread run.
- *thread_out = (pthread_t) thread;
+ // Publish the pthread_t and unlock the mutex to let the new thread start running.
+ *thread_out = reinterpret_cast<pthread_t>(thread);
+ pthread_mutex_unlock(start_mutex);
return 0;
}