From 0f020d18b138e24b1fe34074808e07ac412f35a4 Mon Sep 17 00:00:00 2001
From: msg555 <msg555@gmail.com>
Date: Thu, 6 Jun 2013 14:59:28 -0400
Subject: [PATCH] Handles spurious wake-ups in pthread_join()
Removed 'join_count' from pthread_internal_t and switched to using the flag
PTHREAD_ATTR_FLAG_JOINED to indicate if a thread is being joined. Combined with
a switch to a while loop in pthread_join, this fixes spurious wake-ups but
prevents a thread from being joined multiple times. This is fine for
two reasons:
1) The pthread_join specification allows for undefined behavior when multiple
threads try to join a single thread.
2) There is no thread safe way to allow multiple threads to join a single
thread with the pthread interface. The second thread calling pthread_join
could be pre-empted until the thread is destroyed and its handle reused for
a different thread. Therefore multi-join is always an error.
Bug: https://code.google.com/p/android/issues/detail?id=52255
Change-Id: I8b6784d47620ffdcdbfb14524e7402e21d46c5f7
---
libc/bionic/pthread.c | 22 ++++++----------------
libc/bionic/pthread_create.cpp | 1 -
libc/bionic/pthread_detach.cpp | 2 +-
libc/bionic/pthread_internal.h | 10 +++++++++-
libc/bionic/pthread_join.cpp | 22 ++++++++--------------
libc/bionic/pthread_key.cpp | 9 +++------
tests/pthread_test.cpp | 22 ++++++++++++++++++++++
7 files changed, 49 insertions(+), 39 deletions(-)
diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c
index e30fa9de9..fb1409712 100644
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -130,23 +130,13 @@ void pthread_exit(void * retval)
thread->tls = NULL;
}
- /* the join_count field is used to store the number of threads waiting for
- * the termination of this thread with pthread_join(),
- *
- * if it is positive we need to signal the waiters, and we do not touch
- * the count (it will be decremented by the waiters, the last one will
- * also remove/free the thread structure
- *
- * if it is zero, we set the count value to -1 to indicate that the
- * thread is in 'zombie' state: it has stopped executing, and its stack
- * is gone (as well as its TLS area). when another thread calls pthread_join()
- * on it, it will immediately free the thread and return.
- */
+ /* Indicate that the thread has exited for joining threads. */
+ thread->attr.flags |= PTHREAD_ATTR_FLAG_ZOMBIE;
thread->return_value = retval;
- if (thread->join_count > 0) {
- pthread_cond_broadcast(&thread->join_cond);
- } else {
- thread->join_count = -1; /* zombie thread */
+
+ /* Signal the joining thread if present. */
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+ pthread_cond_signal(&thread->join_cond);
}
}
pthread_mutex_unlock(&gThreadListLock);
diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp
index 70a9bf533..19009e75e 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -110,7 +110,6 @@ int _init_thread(pthread_internal_t* thread, bool add_to_thread_list) {
}
pthread_cond_init(&thread->join_cond, NULL);
- thread->join_count = 0;
thread->cleanup_stack = NULL;
if (add_to_thread_list) {
diff --git a/libc/bionic/pthread_detach.cpp b/libc/bionic/pthread_detach.cpp
index 63f5809d8..95f11ac97 100644
--- a/libc/bionic/pthread_detach.cpp
+++ b/libc/bionic/pthread_detach.cpp
@@ -40,7 +40,7 @@ int pthread_detach(pthread_t t) {
return EINVAL; // Already detached.
}
- if (thread->join_count > 0) {
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
return 0; // Already being joined; silently do nothing, like glibc.
}
diff --git a/libc/bionic/pthread_internal.h b/libc/bionic/pthread_internal.h
index 316a14a28..e34788b3b 100644
--- a/libc/bionic/pthread_internal.h
+++ b/libc/bionic/pthread_internal.h
@@ -42,7 +42,6 @@ typedef struct pthread_internal_t
pid_t tid;
bool allocated_on_heap;
pthread_cond_t join_cond;
- int join_count;
void* return_value;
int internal_flags;
__pthread_cleanup_t* cleanup_stack;
@@ -64,9 +63,18 @@ pthread_internal_t* __get_thread(void);
__LIBC_HIDDEN__ void pthread_key_clean_all(void);
__LIBC_HIDDEN__ void _pthread_internal_remove_locked(pthread_internal_t* thread);
+/* Has the thread been detached by a pthread_join or pthread_detach call? */
#define PTHREAD_ATTR_FLAG_DETACHED 0x00000001
+
+/* Was the thread's stack allocated by the user rather than by us? */
#define PTHREAD_ATTR_FLAG_USER_STACK 0x00000002
+/* Has the thread been joined by another thread? */
+#define PTHREAD_ATTR_FLAG_JOINED 0x00000004
+
+/* Has the thread already exited but not been joined? */
+#define PTHREAD_ATTR_FLAG_ZOMBIE 0x00000008
+
__LIBC_HIDDEN__ extern pthread_internal_t* gThreadList;
__LIBC_HIDDEN__ extern pthread_mutex_t gThreadListLock;
diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp
index e6acc34ce..7e022c224 100644
--- a/libc/bionic/pthread_join.cpp
+++ b/libc/bionic/pthread_join.cpp
@@ -30,7 +30,7 @@
#include "pthread_accessor.h"
-int pthread_join(pthread_t t, void ** ret_val) {
+int pthread_join(pthread_t t, void** ret_val) {
if (t == pthread_self()) {
return EDEADLK;
}
@@ -44,25 +44,19 @@ int pthread_join(pthread_t t, void ** ret_val) {
return EINVAL;
}
- // Wait for thread death when needed.
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_JOINED) {
+ return EINVAL;
+ }
- // If the 'join_count' is negative, this is a 'zombie' thread that
- // is already dead and without stack/TLS. Otherwise, we need to increment 'join-count'
- // and wait to be signaled
- int count = thread->join_count;
- if (count >= 0) {
- thread->join_count += 1;
+ // Signal our intention to join, and wait for the thread to exit.
+ thread->attr.flags |= PTHREAD_ATTR_FLAG_JOINED;
+ while ((thread->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) == 0) {
pthread_cond_wait(&thread->join_cond, &gThreadListLock);
- count = --thread->join_count;
}
if (ret_val) {
*ret_val = thread->return_value;
}
- // Remove thread from thread list when we're the last joiner or when the
- // thread was already a zombie.
- if (count <= 0) {
- _pthread_internal_remove_locked(thread.get());
- }
+ _pthread_internal_remove_locked(thread.get());
return 0;
}
diff --git a/libc/bionic/pthread_key.cpp b/libc/bionic/pthread_key.cpp
index c793fc61c..2ae651919 100644
--- a/libc/bionic/pthread_key.cpp
+++ b/libc/bionic/pthread_key.cpp
@@ -212,16 +212,13 @@ int pthread_key_delete(pthread_key_t key) {
// Clear value in all threads.
pthread_mutex_lock(&gThreadListLock);
for (pthread_internal_t* t = gThreadList; t != NULL; t = t->next) {
- // Avoid zombie threads with a negative 'join_count'. These are really
- // already dead and don't have a TLS area anymore.
-
+ // Skip zombie threads. They don't have a valid TLS area any more.
// Similarly, it is possible to have t->tls == NULL for threads that
// were just recently created through pthread_create() but whose
// startup trampoline (__thread_entry) hasn't been run yet by the
- // scheduler. t->tls will also be NULL after it's stack has been
+ // scheduler. t->tls will also be NULL after a thread's stack has been
// unmapped but before the ongoing pthread_join() is finished.
- // so check for this too.
- if (t->join_count < 0 || !t->tls) {
+ if ((t->attr.flags & PTHREAD_ATTR_FLAG_ZOMBIE) || t->tls == NULL) {
continue;
}
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index a86cadc6b..d754312f9 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -317,3 +317,25 @@ TEST(pthread, pthread_kill__no_such_thread) {
ASSERT_EQ(ESRCH, pthread_kill(dead_thread, 0));
}
+
+TEST(pthread, pthread_join__multijoin) {
+ bool done = false;
+
+ pthread_t t1;
+ ASSERT_EQ(0, pthread_create(&t1, NULL, SpinFn, &done));
+
+ pthread_t t2;
+ ASSERT_EQ(0, pthread_create(&t2, NULL, JoinFn, reinterpret_cast<void*>(t1)));
+
+ sleep(1); // (Give t2 a chance to call pthread_join.)
+
+ // Multiple joins to the same thread should fail.
+ ASSERT_EQ(EINVAL, pthread_join(t1, NULL));
+
+ done = true;
+
+ // ...but t2's join on t1 still goes ahead (which we can tell because our join on t2 finishes).
+ void* join_result;
+ ASSERT_EQ(0, pthread_join(t2, &join_result));
+ ASSERT_EQ(0, reinterpret_cast<int>(join_result));
+}