diff --git a/libc/bionic/pthread_create.cpp b/libc/bionic/pthread_create.cpp index bfa4e8ce0..ab9285368 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 2bf200480..f641e4c6a 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(thread->tid) << 3; + clockid_t result = ~static_cast(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 052fb05f9..cc1ece8ed 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 c996a0522..fe854420d 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(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 8946f79af..51430e772 100644 --- a/libc/bionic/pthread_internal.cpp +++ b/libc/bionic/pthread_internal.cpp @@ -34,20 +34,38 @@ #include #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 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 ScopedWriteLock; +typedef ScopedRWLock 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 @@ pthread_t __pthread_internal_add(pthread_internal_t* thread) { } 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 @@ void __pthread_internal_remove_and_free(pthread_internal_t* thread) { pthread_internal_t* __pthread_internal_find(pthread_t thread_id) { pthread_internal_t* thread = reinterpret_cast(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 72a6ed1c1..3761a7589 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 6d2880eaf..f582d5386 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 0ad68bb38..3e8095956 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 b0c95fef4..bf86f5b18 100755 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -443,14 +443,20 @@ TEST(pthread, pthread_setname_np__pthread_getname_np__other_PR_SET_DUMPABLE) { 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); + + 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); - // Call pthread_getname_np and pthread_setname_np after the thread has already exited. - ASSERT_EQ(ENOENT, pthread_setname_np(dead_thread, "short 3")); 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 @@ TEST(pthread, pthread_kill__in_signal_handler) { 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 @@ TEST(pthread, pthread_getcpuclockid__clock_gettime) { 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 8c4a8a9e3..4e3fa834c 100644 --- a/tests/time_test.cpp +++ b/tests/time_test.cpp @@ -509,14 +509,14 @@ TEST(time, timer_delete_terminates) { 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(value.sival_ptr); - tdd->thread_id = pthread_self(); + tdd->tid = gettid(); timer_delete(tdd->timer_id); tdd->complete = true; } @@ -548,8 +548,9 @@ TEST(time, timer_delete_from_timer_thread) { // 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 }