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_create.cpp b/libc/bionic/pthread_create.cpp
index bfa4e8c..ab92853 100644
--- a/libc/bionic/pthread_create.cpp
+++ b/libc/bionic/pthread_create.cpp
@@ -41,7 +41,6 @@
#include "private/bionic_tls.h"
#include "private/libc_logging.h"
#include "private/ErrnoRestorer.h"
-#include "private/ScopedPthreadMutexLocker.h"
// x86 uses segment descriptors rather than a direct pointer to TLS.
#if defined(__i386__)
diff --git a/libc/bionic/pthread_getcpuclockid.cpp b/libc/bionic/pthread_getcpuclockid.cpp
index 2bf2004..f641e4c 100644
--- a/libc/bionic/pthread_getcpuclockid.cpp
+++ b/libc/bionic/pthread_getcpuclockid.cpp
@@ -31,13 +31,11 @@
#include "pthread_internal.h"
int pthread_getcpuclockid(pthread_t t, clockid_t* clockid) {
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == NULL) {
- return ESRCH;
- }
+ pid_t tid = pthread_gettid_np(t);
+ if (tid == -1) return ESRCH;
// The tid is stored in the top bits, but negated.
- clockid_t result = ~static_cast<clockid_t>(thread->tid) << 3;
+ clockid_t result = ~static_cast<clockid_t>(tid) << 3;
// Bits 0 and 1: clock type (0 = CPUCLOCK_PROF, 1 = CPUCLOCK_VIRT, 2 = CPUCLOCK_SCHED).
result |= 2;
// Bit 2: thread (set) or process (clear)?
diff --git a/libc/bionic/pthread_getschedparam.cpp b/libc/bionic/pthread_getschedparam.cpp
index 052fb05..cc1ece8 100644
--- a/libc/bionic/pthread_getschedparam.cpp
+++ b/libc/bionic/pthread_getschedparam.cpp
@@ -34,15 +34,10 @@
int pthread_getschedparam(pthread_t t, int* policy, sched_param* param) {
ErrnoRestorer errno_restorer;
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == NULL) {
- return ESRCH;
- }
+ pid_t tid = pthread_gettid_np(t);
+ if (tid == -1) return ESRCH;
- int rc = sched_getparam(thread->tid, param);
- if (rc == -1) {
- return errno;
- }
- *policy = sched_getscheduler(thread->tid);
+ if (sched_getparam(tid, param) == -1) return errno;
+ *policy = sched_getscheduler(tid);
return 0;
}
diff --git a/libc/bionic/pthread_gettid_np.cpp b/libc/bionic/pthread_gettid_np.cpp
index c996a05..fe85442 100644
--- a/libc/bionic/pthread_gettid_np.cpp
+++ b/libc/bionic/pthread_gettid_np.cpp
@@ -29,5 +29,6 @@
#include "pthread_internal.h"
pid_t pthread_gettid_np(pthread_t t) {
- return reinterpret_cast<pthread_internal_t*>(t)->tid;
+ pthread_internal_t* thread = __pthread_internal_find(t);
+ return thread ? thread->tid : -1;
}
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;
}
diff --git a/libc/bionic/pthread_kill.cpp b/libc/bionic/pthread_kill.cpp
index 72a6ed1..3761a75 100644
--- a/libc/bionic/pthread_kill.cpp
+++ b/libc/bionic/pthread_kill.cpp
@@ -35,10 +35,8 @@
int pthread_kill(pthread_t t, int sig) {
ErrnoRestorer errno_restorer;
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == NULL) {
- return ESRCH;
- }
+ pid_t tid = pthread_gettid_np(t);
+ if (tid == -1) return ESRCH;
- return (tgkill(getpid(), thread->tid, sig) == -1) ? errno : 0;
+ return (tgkill(getpid(), tid, sig) == -1) ? errno : 0;
}
diff --git a/libc/bionic/pthread_setname_np.cpp b/libc/bionic/pthread_setname_np.cpp
index 6d2880e..f582d53 100644
--- a/libc/bionic/pthread_setname_np.cpp
+++ b/libc/bionic/pthread_setname_np.cpp
@@ -43,14 +43,8 @@
#define MAX_TASK_COMM_LEN 16
static int __open_task_comm_fd(pthread_t t, int flags) {
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == nullptr) {
- errno = ENOENT;
- return -1;
- }
-
char comm_name[64];
- snprintf(comm_name, sizeof(comm_name), "/proc/self/task/%d/comm", thread->tid);
+ snprintf(comm_name, sizeof(comm_name), "/proc/self/task/%d/comm", pthread_gettid_np(t));
return open(comm_name, O_CLOEXEC | flags);
}
diff --git a/libc/bionic/pthread_setschedparam.cpp b/libc/bionic/pthread_setschedparam.cpp
index 0ad68bb..3e80959 100644
--- a/libc/bionic/pthread_setschedparam.cpp
+++ b/libc/bionic/pthread_setschedparam.cpp
@@ -34,14 +34,8 @@
int pthread_setschedparam(pthread_t t, int policy, const sched_param* param) {
ErrnoRestorer errno_restorer;
- pthread_internal_t* thread = __pthread_internal_find(t);
- if (thread == NULL) {
- return ESRCH;
- }
+ pid_t tid = pthread_gettid_np(t);
+ if (tid == -1) return ESRCH;
- int rc = sched_setscheduler(thread->tid, policy, param);
- if (rc == -1) {
- return errno;
- }
- return 0;
+ return (sched_setscheduler(tid, policy, param) == -1) ? errno : 0;
}