Be more strict about using invalid `pthread_t`s.
Another release, another attempt to remove the global thread list.
But this time, let's admit that it's not going away. We can switch to using
a read/write lock for the global thread list, and to aborting rather than
quietly returning ESRCH if we're given an invalid pthread_t.
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, if you're
targeting O or above, they'll abort with the message "attempt to use
invalid pthread_t".
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.
(This patch replaces such users with calls to pthread_gettid_np, which
at least makes the TOCTOU window smaller.)
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.
Bug: http://b/19636317
Test: ran tests
Change-Id: I0372c4428e8a7f1c3af5c9334f5d9c25f2c73f21
diff --git a/libc/bionic/pthread_internal.cpp b/libc/bionic/pthread_internal.cpp
index 8946f79..51430e7 100644
--- a/libc/bionic/pthread_internal.cpp
+++ b/libc/bionic/pthread_internal.cpp
@@ -34,20 +34,38 @@
#include <sys/mman.h>
#include "private/bionic_futex.h"
+#include "private/bionic_sdk_version.h"
#include "private/bionic_tls.h"
#include "private/libc_logging.h"
-#include "private/ScopedPthreadMutexLocker.h"
-static pthread_internal_t* g_thread_list = NULL;
-static pthread_mutex_t g_thread_list_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_internal_t* g_thread_list = nullptr;
+static pthread_rwlock_t g_thread_list_lock = PTHREAD_RWLOCK_INITIALIZER;
+
+template <bool write> class ScopedRWLock {
+ public:
+ ScopedRWLock(pthread_rwlock_t* rwlock) : rwlock_(rwlock) {
+ (write ? pthread_rwlock_wrlock : pthread_rwlock_rdlock)(rwlock_);
+ }
+
+ ~ScopedRWLock() {
+ pthread_rwlock_unlock(rwlock_);
+ }
+
+ private:
+ pthread_rwlock_t* rwlock_;
+ DISALLOW_IMPLICIT_CONSTRUCTORS(ScopedRWLock);
+};
+
+typedef ScopedRWLock<true> ScopedWriteLock;
+typedef ScopedRWLock<false> ScopedReadLock;
pthread_t __pthread_internal_add(pthread_internal_t* thread) {
- ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+ ScopedWriteLock locker(&g_thread_list_lock);
// We insert at the head.
thread->next = g_thread_list;
- thread->prev = NULL;
- if (thread->next != NULL) {
+ thread->prev = nullptr;
+ if (thread->next != nullptr) {
thread->next->prev = thread;
}
g_thread_list = thread;
@@ -55,12 +73,12 @@
}
void __pthread_internal_remove(pthread_internal_t* thread) {
- ScopedPthreadMutexLocker locker(&g_thread_list_lock);
+ ScopedWriteLock locker(&g_thread_list_lock);
- if (thread->next != NULL) {
+ if (thread->next != nullptr) {
thread->next->prev = thread->prev;
}
- if (thread->prev != NULL) {
+ if (thread->prev != nullptr) {
thread->prev->next = thread->next;
} else {
g_thread_list = thread->next;
@@ -82,17 +100,17 @@
pthread_internal_t* __pthread_internal_find(pthread_t thread_id) {
pthread_internal_t* thread = reinterpret_cast<pthread_internal_t*>(thread_id);
- // check if thread is pthread_self() before acquiring the lock
- if (thread == __get_thread()) {
- return thread;
+ // Check if we're looking for ourselves before acquiring the lock.
+ if (thread == __get_thread()) return thread;
+
+ ScopedReadLock locker(&g_thread_list_lock);
+ for (pthread_internal_t* t = g_thread_list; t != nullptr; t = t->next) {
+ if (t == thread) return thread;
}
- ScopedPthreadMutexLocker locker(&g_thread_list_lock);
-
- for (pthread_internal_t* t = g_thread_list; t != NULL; t = t->next) {
- if (t == thread) {
- return thread;
- }
+ // Historically we'd return null, but
+ if (bionic_get_application_target_sdk_version() >= __ANDROID_API_O__) {
+ __libc_fatal("attempt to use invalid pthread_t");
}
- return NULL;
+ return nullptr;
}