Fix bug in pthread_join, pthread_exit, pthread_detach
pthread_no_op_detach_after_join test from bionic-unit-tests hangs
on x86 emulator. There is a race in the pthread_join, pthread_exit,
pthread_detach functions:
- pthread_join waits for the non-detached thread
- pthread_detach sets the detached flag on that thread
- the thread executes pthread_exit which just kills the now-detached
thread, without sending the join notification.
This patch improves the test so it fails on ARM too, and modifies
pthread_detach to behave more like glibc, not setting the detach state if
called on a thread that's already being joined (but not returning an error).
Change-Id: I87dc688221ce979ef5178753dd63d01ac0b108e6
Signed-off-by: Sergey Melnikov <sergey.melnikov@intel.com>
diff --git a/libc/bionic/pthread.c b/libc/bionic/pthread.c
index c7612f7..791a772 100644
--- a/libc/bionic/pthread.c
+++ b/libc/bionic/pthread.c
@@ -582,7 +582,7 @@
pthread_key_clean_all();
// if the thread is detached, destroy the pthread_internal_t
- // otherwise, keep it in memory and signal any joiners
+ // 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);
@@ -668,8 +668,9 @@
pthread_cond_wait( &thread->join_cond, &gThreadListLock );
count = --thread->join_count;
}
- if (ret_val)
+ if (ret_val) {
*ret_val = thread->return_value;
+ }
/* remove thread descriptor when we're the last joiner or when the
* thread was already a zombie.
@@ -686,28 +687,30 @@
{
pthread_internal_t* thread;
int result = 0;
- int flags;
pthread_mutex_lock(&gThreadListLock);
- for (thread = gThreadList; thread != NULL; thread = thread->next)
- if (thread == (pthread_internal_t*)thid)
+ for (thread = gThreadList; thread != NULL; thread = thread->next) {
+ if (thread == (pthread_internal_t*)thid) {
goto FoundIt;
+ }
+ }
result = ESRCH;
goto Exit;
FoundIt:
- do {
- flags = thread->attr.flags;
-
- if ( flags & PTHREAD_ATTR_FLAG_DETACHED ) {
- /* thread is not joinable ! */
- result = EINVAL;
- goto Exit;
- }
+ if (thread->attr.flags & PTHREAD_ATTR_FLAG_DETACHED) {
+ result = EINVAL; // Already detached.
+ goto Exit;
}
- while ( __bionic_cmpxchg( flags, flags | PTHREAD_ATTR_FLAG_DETACHED,
- (volatile int*)&thread->attr.flags ) != 0 );
+
+ if (thread->join_count > 0) {
+ result = 0; // Already being joined; silently do nothing, like glibc.
+ goto Exit;
+ }
+
+ thread->attr.flags |= PTHREAD_ATTR_FLAG_DETACHED;
+
Exit:
pthread_mutex_unlock(&gThreadListLock);
return result;
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index 479c462..e400b84 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -37,10 +37,26 @@
return NULL;
}
+static void* SpinFn(void* arg) {
+ volatile bool* b = reinterpret_cast<volatile bool*>(arg);
+ while (!*b) {
+ }
+ return NULL;
+}
+
static void* JoinFn(void* arg) {
return reinterpret_cast<void*>(pthread_join(reinterpret_cast<pthread_t>(arg), NULL));
}
+static void AssertDetached(pthread_t t, bool is_detached) {
+ pthread_attr_t attr;
+ ASSERT_EQ(0, pthread_getattr_np(t, &attr));
+ int detach_state;
+ ASSERT_EQ(0, pthread_attr_getdetachstate(&attr, &detach_state));
+ pthread_attr_destroy(&attr);
+ ASSERT_EQ(is_detached, (detach_state == PTHREAD_CREATE_DETACHED));
+}
+
TEST(pthread, pthread_create) {
void* expected_result = reinterpret_cast<void*>(123);
// Can we create a thread?
@@ -58,6 +74,7 @@
// After a pthread_detach...
ASSERT_EQ(0, pthread_detach(t1));
+ AssertDetached(t1, true);
// ...pthread_join should fail.
void* result;
@@ -65,20 +82,27 @@
}
TEST(pthread, pthread_no_op_detach_after_join) {
+ bool done = false;
+
pthread_t t1;
- ASSERT_EQ(0, pthread_create(&t1, NULL, SleepFn, reinterpret_cast<void*>(1)));
+ ASSERT_EQ(0, pthread_create(&t1, NULL, SpinFn, &done));
// If thread 2 is already waiting to join thread 1...
pthread_t t2;
ASSERT_EQ(0, pthread_create(&t2, NULL, JoinFn, reinterpret_cast<void*>(t1)));
- // ...a call to pthread_detach on thread 1 will "succeed"...
- ASSERT_EQ(0, pthread_detach(t1));
+ sleep(1); // (Give t2 a chance to call pthread_join.)
- // ...but the join still goes ahead.
+ // ...a call to pthread_detach on thread 1 will "succeed" (silently fail)...
+ ASSERT_EQ(0, pthread_detach(t1));
+ AssertDetached(t1, false);
+
+ 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(EINVAL, reinterpret_cast<int>(join_result));
+ ASSERT_EQ(0, reinterpret_cast<int>(join_result));
}
TEST(pthread, pthread_join_self) {
