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;
}
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index b0c95fe..bf86f5b 100755
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -443,14 +443,20 @@
ASSERT_EQ(0, pthread_join(t, nullptr));
}
-TEST(pthread, pthread_setname_np__pthread_getname_np__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_setname_np__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
- // Call pthread_getname_np and pthread_setname_np after the thread has already exited.
- ASSERT_EQ(ENOENT, pthread_setname_np(dead_thread, "short 3"));
+ EXPECT_DEATH(pthread_setname_np(dead_thread, "short 3"), "attempt to use invalid pthread_t");
+}
+
+TEST_F(pthread_DeathTest, pthread_getname_np__no_such_thread) {
+ pthread_t dead_thread;
+ MakeDeadThread(dead_thread);
+
char name[64];
- ASSERT_EQ(ENOENT, pthread_getname_np(dead_thread, name, sizeof(name)));
+ EXPECT_DEATH(pthread_getname_np(dead_thread, name, sizeof(name)),
+ "attempt to use invalid pthread_t");
}
TEST(pthread, pthread_kill__0) {
@@ -476,11 +482,11 @@
ASSERT_EQ(0, pthread_kill(pthread_self(), SIGALRM));
}
-TEST(pthread, pthread_detach__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_detach__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
- ASSERT_EQ(ESRCH, pthread_detach(dead_thread));
+ EXPECT_DEATH(pthread_detach(dead_thread), "attempt to use invalid pthread_t");
}
TEST(pthread, pthread_getcpuclockid__clock_gettime) {
@@ -497,44 +503,46 @@
ASSERT_EQ(0, pthread_join(t, nullptr));
}
-TEST(pthread, pthread_getcpuclockid__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_getcpuclockid__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
clockid_t c;
- ASSERT_EQ(ESRCH, pthread_getcpuclockid(dead_thread, &c));
+ EXPECT_DEATH(pthread_getcpuclockid(dead_thread, &c), "attempt to use invalid pthread_t");
}
-TEST(pthread, pthread_getschedparam__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_getschedparam__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
int policy;
sched_param param;
- ASSERT_EQ(ESRCH, pthread_getschedparam(dead_thread, &policy, ¶m));
+ EXPECT_DEATH(pthread_getschedparam(dead_thread, &policy, ¶m),
+ "attempt to use invalid pthread_t");
}
-TEST(pthread, pthread_setschedparam__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_setschedparam__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
int policy = 0;
sched_param param;
- ASSERT_EQ(ESRCH, pthread_setschedparam(dead_thread, policy, ¶m));
+ EXPECT_DEATH(pthread_setschedparam(dead_thread, policy, ¶m),
+ "attempt to use invalid pthread_t");
}
-TEST(pthread, pthread_join__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_join__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
- ASSERT_EQ(ESRCH, pthread_join(dead_thread, NULL));
+ EXPECT_DEATH(pthread_join(dead_thread, NULL), "attempt to use invalid pthread_t");
}
-TEST(pthread, pthread_kill__no_such_thread) {
+TEST_F(pthread_DeathTest, pthread_kill__no_such_thread) {
pthread_t dead_thread;
MakeDeadThread(dead_thread);
- ASSERT_EQ(ESRCH, pthread_kill(dead_thread, 0));
+ EXPECT_DEATH(pthread_kill(dead_thread, 0), "attempt to use invalid pthread_t");
}
TEST(pthread, pthread_join__multijoin) {
diff --git a/tests/time_test.cpp b/tests/time_test.cpp
index 8c4a8a9..4e3fa83 100644
--- a/tests/time_test.cpp
+++ b/tests/time_test.cpp
@@ -509,14 +509,14 @@
struct TimerDeleteData {
timer_t timer_id;
- pthread_t thread_id;
+ pid_t tid;
volatile bool complete;
};
static void TimerDeleteCallback(sigval_t value) {
TimerDeleteData* tdd = reinterpret_cast<TimerDeleteData*>(value.sival_ptr);
- tdd->thread_id = pthread_self();
+ tdd->tid = gettid();
timer_delete(tdd->timer_id);
tdd->complete = true;
}
@@ -548,8 +548,9 @@
// Since bionic timers are implemented by creating a thread to handle the
// callback, verify that the thread actually completes.
cur_time = time(NULL);
- while (pthread_detach(tdd.thread_id) != ESRCH && (time(NULL) - cur_time) < 5);
- ASSERT_EQ(ESRCH, pthread_detach(tdd.thread_id));
+ while ((kill(tdd.tid, 0) != -1 || errno != ESRCH) && (time(NULL) - cur_time) < 5);
+ ASSERT_EQ(-1, kill(tdd.tid, 0));
+ ASSERT_EQ(ESRCH, errno);
#endif
}