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/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) {