Remove the global thread list.
Another release, another attempt to fix this bug.
This change affects pthread_detach, pthread_getcpuclockid,
pthread_getschedparam/pthread_setschedparam, pthread_join, and pthread_kill:
instead of returning ESRCH when passed an invalid pthread_t, they'll now SEGV.
Note that this doesn't change behavior as much as you might think: the old
lookup only held the global thread list lock for the duration of the lookup,
so there was still a race between that and the dereference in the caller,
given that callers actually need the tid to pass to some syscall or other,
and sometimes update fields in the pthread_internal_t struct too.
We can't check thread->tid against 0 to see whether a pthread_t is still
valid because a dead thread gets its thread struct unmapped along with its
stack, so the dereference isn't safe.
Taking the affected functions one by one:
* pthread_getcpuclockid and pthread_getschedparam/pthread_setschedparam
should be fine. Unsafe calls to those seem highly unlikely.
* Unsafe pthread_detach callers probably want to switch to
pthread_attr_setdetachstate instead, or using pthread_detach(pthread_self())
from the new thread's start routine rather than doing the detach in the
parent.
* pthread_join calls should be safe anyway, because a joinable thread won't
actually exit and unmap until it's joined. If you're joining an
unjoinable thread, the fix is to stop marking it detached. If you're
joining an already-joined thread, you need to rethink your design.
* Unsafe pthread_kill calls aren't portably fixable. (And are obviously
inherently non-portable as-is.) The best alternative on Android is to
use pthread_gettid_np at some point that you know the thread to be alive,
and then call kill/tgkill directly. That's still not completely safe
because if you're too late, the tid may have been reused, but then your
code is inherently unsafe anyway.
If we find too much code is still broken, we can come back and disable
the global thread list lookups for anything targeting >= O and then have
another go at really removing this in P...
Bug: http://b/19636317
Test: N6P boots, bionic tests pass
Change-Id: Ia92641212f509344b99ee2a9bfab5383147fcba6
diff --git a/libc/bionic/pthread_join.cpp b/libc/bionic/pthread_join.cpp
index 4d852cb..d61a096 100644
--- a/libc/bionic/pthread_join.cpp
+++ b/libc/bionic/pthread_join.cpp
@@ -36,10 +36,7 @@
return EDEADLK;
}
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == NULL) {
- return ESRCH;
- }
+ pthread_internal_t* thread = reinterpret_cast<pthread_internal_t*>(t);
ThreadJoinState old_state = THREAD_NOT_JOINED;
while ((old_state == THREAD_NOT_JOINED || old_state == THREAD_EXITED_NOT_JOINED) &&
@@ -65,6 +62,6 @@
*return_value = thread->return_value;
}
- __pthread_internal_remove_and_free(thread);
+ __free_thread(thread);
return 0;
}